Artifact uri phase 3 cli runtime variable contract exec#267
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBrings branded RunId (rd_... 32-hex) and structured RunbookRef (source+path) into core types/schemas, makes preparation/launch identity-aware (RunbookRef/RunId), propagates RunbookRef through delegation/claim/session flows, updates variable-discovery/reserved names and WorkPath behavior, and synchronizes CLI, docs, and tests to the new model and naming conventions. ChangesRunbook Identity & Execution DAG
Sequence Diagram(s)sequenceDiagram
participant CLI as User/CLI
participant Resolve as resolveRunbookFile / resolveRunbookRef
participant Prep as prepareRunnableRunbook
participant Manager as RunbookStateManager
participant Session as SessionService
participant Exec as Execution Loop
CLI->>Resolve: resolveRunbookFile(path)
Resolve-->>CLI: ResolvedRunbook { path, source, sourceRoot }
CLI->>Prep: prepareRunnableRunbook(resolved, inputs, cwd)
Prep->>Prep: buildRunbookRef(resolved)
Prep->>Prep: generateRunId() -> rd_<...>
Prep-->>CLI: RunnableRunbook { runId, runbookRef, mergedVariables }
CLI->>Manager: create(runbookRef, runbook, { runId? })
Manager->>Manager: persist RunbookState { id: RunId, runbook: RunbookRef, ... }
Manager-->>CLI: RunbookState
CLI->>Exec: launch(RunnableRunbook)
Exec->>Exec: inject RD_RUN_ID, RD_RUNBOOK_REF, RD_RUNBOOK_SOURCE, RD_CONTEXT_ID
Exec->>Exec: execute steps
sequenceDiagram
participant Parent as Parent Runbook
participant DelegateCmd as delegate command
participant Resolve as resolveRunbookFile
participant Build as buildRunbookRef
participant DelegSvc as createDelegation
participant Session as SessionService
participant Child as Child Claim & Launch
Parent->>DelegateCmd: delegate child-spec
DelegateCmd->>Resolve: resolveRunbookFile(child-spec)
Resolve-->>DelegateCmd: ResolvedRunbook {path, source, sourceRoot}
DelegateCmd->>Build: buildRunbookRef(resolved)
Build-->>DelegateCmd: RunbookRef {source,path}
DelegateCmd->>DelegSvc: createDelegation(..., childRunbookRef)
DelegSvc->>Session: persist delegation with childRunbookRef
Note over Parent,Child: Later
Child->>Session: claim delegation (token)
Session-->>Child: claim record → uses childRunbookRef to prepare child
Child->>Prep: prepareRunnableRunbook(using childRunbookRef)
Prep->>Prep: generateRunId() for child run
Child->>Child: launch child with fresh RunId and RunbookRef
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Deploying rundown with
|
| Latest commit: |
cee131c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7ba2ef0a.rundown-7hl.pages.dev |
| Branch Preview URL: | https://artifact-uri-phase-3-cli-run-p4pq.rundown-7hl.pages.dev |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bca72ef924
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/cli/src/services/variable-discovery.ts (1)
34-44: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winReturn a disposer function from
setExecFileSyncImpl()for deterministic cleanup.Current tests correctly restore via
afterEach()within thegetBuiltinVariablesscope, but this pattern depends on developer discipline. Jest runs tests in parallel by default (norunInBandconfig), so the module-globalexecFileSyncImplis inherently racy without explicit restoration. A disposer pattern makes cleanup self-documenting and prevents accidental leaks if new test blocks are added without correspondingafterEachhooks.♻️ Proposed change
-export function setExecFileSyncImpl(fn: typeof nodeExecFileSync): void { - execFileSyncImpl = fn; +export function setExecFileSyncImpl(fn: typeof nodeExecFileSync): () => void { + const previous = execFileSyncImpl; + execFileSyncImpl = fn; + return () => { + execFileSyncImpl = previous; + }; }Update test calls to use the returned restore function:
it('should return fixed WorkPath even when in git repo', () => { - setExecFileSyncImpl((() => 'feature/my-branch\n') as unknown as typeof nodeExecFileSync); + const restore = setExecFileSyncImpl((() => 'feature/my-branch\n') as unknown as typeof nodeExecFileSync); const builtins = getBuiltinVariables(); expect(builtins.WorkPath).toBe(WORK_DIR); expect(builtins.Branch).toBe('feature/my-branch'); + restore(); });Or use in
beforeEach/afterEachfor cleaner test structure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/services/variable-discovery.ts` around lines 34 - 44, The setExecFileSyncImpl currently replaces the module-global execFileSyncImpl but does not return a disposer; change setExecFileSyncImpl(fn: typeof nodeExecFileSync) to capture the current execFileSyncImpl, assign the new fn to execFileSyncImpl, and return a restore function that when called reassigns the captured original back to execFileSyncImpl so tests can deterministically restore the previous implementation; reference the module-level variable execFileSyncImpl and the exported function setExecFileSyncImpl when making this change.packages/claude-code-plugin/__tests__/rdpath-find-integration.test.ts (1)
393-408:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix error message pattern mismatch in recovery classification for invalid session data.
Tests at lines 393-408, 433-451, and 453-468 fail with
exitCode: 1when0is expected. The soft-fail contract—gracefully ignoring stale or invalid active-runbook state when--dirorRD_WORK_PATHis explicitly provided—is broken.Root cause:
isRecoverableActiveStateLookupError()checks for'Session file contains invalid entries'(line 73), butRunbookStateManager.loadSession()throws with'Session file contains invalid runbook targeting data'whenSessionDataSchemavalidation fails on invalid IDs like../outside. The string mismatch causes the error to be treated as non-recoverable, so it propagates instead of being caught and silently skipped.Fix: Update the recovery pattern at line 73 to match the actual error message, or update the core error message to match the pattern. Either way, this pattern mismatch must be resolved to restore the documented soft-fail behavior for stale/invalid state when an explicit path is provided.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/claude-code-plugin/__tests__/rdpath-find-integration.test.ts` around lines 393 - 408, The recovery matcher in isRecoverableActiveStateLookupError incorrectly looks for 'Session file contains invalid entries' while RunbookStateManager.loadSession throws 'Session file contains invalid runbook targeting data'; update isRecoverableActiveStateLookupError to match the actual thrown message (or use a regex/contains check that covers both 'invalid entries' and 'invalid runbook targeting data') so the error is classified as recoverable and the code paths that silently ignore stale/invalid session state when --dir or RD_WORK_PATH is provided will execute.packages/core/src/runbook/delegation-service.ts (1)
679-685:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject stale delegations that predate
childRunbookRef.
retryDelegation()now treatschildRunbookRefas required, but this branch never rejects an older persisted delegation that lacks it. In that case, the retry path will mint a replacement delegation without a canonical child ref instead of failing closed and telling the operator to restart/prune stale state.Suggested guard
+ if (existingDelegation.childRunbookRef === undefined) { + return { + status: 'error', + error: Errors.delegationSnapshotStale(substepId, state.step), + }; + } + const createResult: CreateDelegationResult = createDelegation( { state: stateAfterAbort, stepId: stepIdForCreate, childRunbookPath: existingDelegation.childRunbookPath, childRunbookRef: existingDelegation.childRunbookRef,Based on learnings: when evolving persisted state schemas in the core package, stale or incompatible state must be rejected rather than migrated or reconstructed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/runbook/delegation-service.ts` around lines 679 - 685, The branch that constructs createResult must reject stale persisted delegations that lack a canonical childRunbookRef instead of reconstructing them: in retryDelegation() add a guard that checks existingDelegation.childRunbookRef is present (and, if applicable, matches any expected ref) and if it's missing, return/throw a failure indicating stale/incompatible state rather than calling createDelegation; reference the retryDelegation function, existingDelegation, childRunbookRef, and the createDelegation/CreateDelegationResult flow so the code aborts early for stale delegations.packages/cli/__tests__/helpers/runbook-pipeline.test.ts (1)
303-317: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse canonical
RunIdfixtures inmakeState.
makeState()still accepts arbitrary IDs, so a lot of these new identity-path tests run against values likeparent-idandorphan-idinstead of therd_<32 hex>contract this PR is enforcing. That makes it easy for persisted-state and claim-flow regressions to slip through without failing the suite.🔧 Suggested fixture tightening
-function makeState(id: string, overrides: Record<string, unknown> = {}): Record<string, unknown> { +function makeState( + id: RunId = brandRunIdForTest(`rd_${'b'.repeat(32)}`), + overrides: Record<string, unknown> = {}, +): Record<string, unknown> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/__tests__/helpers/runbook-pipeline.test.ts` around lines 303 - 317, The tests use makeState(id, ...) to build run state but pass arbitrary ids like "parent-id" instead of canonical RunId strings; update makeState to accept or default to a valid RunId (format "rd_<32 hex>") or call the existing RunId fixture generator so all created states use the rd_<32 hex> contract; locate makeState in packages/cli/__tests__/helpers/runbook-pipeline.test.ts and either change its default id to a canonical RunId or wrap incoming ids with the project’s RunId factory/util (ensuring tests that intentionally pass invalid ids remain explicit), then run the suite to verify identity-path tests exercise the real RunId format.packages/cli/__tests__/commands/delegate.test.ts (1)
161-167: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAssert the persisted
childRunbookRefhere too.This now locks down the new
childRunIdformat, but it still doesn't verify the new structured runbook identity. Ifrd delegateregressed to persisting onlychildRunbookPath, this test would still pass.🔧 Suggested assertion
const ss1 = substepStates?.find((ss) => ss.id === '1'); const delegation = ss1?.delegation as Record<string, unknown>; + expect(delegation.childRunbookRef).toEqual({ + source: 'project', + path: 'child.runbook.md', + }); expect(delegation.childRunId).toEqual(expect.stringMatching(/^rd_[a-f0-9]{32}$/)); expect(delegation.tokenHash).toEqual(expect.stringMatching(/^sha256:[a-f0-9]{64}$/));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/__tests__/commands/delegate.test.ts` around lines 161 - 167, Add an assertion to this test to verify the persisted structured runbook identity: after obtaining delegation from ss1 (using getActiveState, substepStates, ss1, delegation), assert that delegation.childRunbookRef exists and matches the expected structure (e.g., contains the runbook id/ref format your code uses) — this ensures we validate childRunbookRef is persisted (not just childRunbookPath); update the expectation alongside the existing childRunId/tokenHash/token checks to assert the exact shape or required fields of childRunbookRef.packages/cli/src/helpers/goto-workflow.ts (1)
84-91: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueType constraint loosened from
RunbookState['id']tostring.With the PR introducing branded
RunIdtypes, this change accepts any string rather than enforcing theRunIdbrand. If callers already have aRunbookStatereference, consider keeping the stronger type or acceptingRunIddirectly for compile-time validation of ID provenance.If this is a deliberate CLI-boundary decision (user input strings), document that the caller is responsible for validating the ID format.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/helpers/goto-workflow.ts` around lines 84 - 91, The function resolveTerminalReleaseModeForRunbook has loosened the runbookId parameter to plain string, weakening the RunbookState['id'] (or branded RunId) type guarantee; restore the stronger type by changing the runbookId parameter back to RunbookState['id'] or the RunId brand (instead of string) in the function signature and adjust any call sites to pass the branded id, or if this is intentionally a CLI boundary accept string but add a short doc comment on resolveTerminalReleaseModeForRunbook stating callers must validate/parse the RunId format before calling and optionally perform a runtime validation in the function (use manager.loadSession() and the existing claim check) to assert the id shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/__tests__/commands/output-format.test.ts`:
- Around line 4-8: Remove the locally defined RUN_ID_PATTERN and import the
canonical RUN_ID_PATTERN instead, then update expectRunId to use the imported
symbol; specifically, delete the local const RUN_ID_PATTERN, add an import for
RUN_ID_PATTERN from the central core source at the top of the test file, and
ensure function expectRunId(text: string) calls expect(text).toMatch(new
RegExp(`\\b${RUN_ID_PATTERN.source}\\b`)) using that imported symbol.
In `@packages/cli/__tests__/helpers/delegate-inference.test.ts`:
- Around line 48-49: The test state currently sets runbook.path and runbookPath
independently which can lead to inconsistent states; update the makeState helper
(function makeState) so runbookPath is derived from runbook.path by default
(i.e., if runbookPath is not explicitly provided, set it to the value of
runbook.path), and ensure any callers that pass runbook.path but omit
runbookPath still produce a synchronized value; reference the runbook.path and
runbookPath fields in makeState to implement this defaulting behavior.
- Line 47: Replace the casted fixture id value that uses "test-run-id" as
RunbookState['id'] with a realistic runtime-style id starting with the rd_
prefix so prefix-dependent logic is exercised; locate the test fixture object in
delegate-inference.test.ts where the property id is assigned (the line
containing id: 'test-run-id' as RunbookState['id']) and change the value to a
string like "rd_<something>" that conforms to the RunId contract instead of
casting, ensuring all dependent assertions still pass.
In `@packages/cli/__tests__/helpers/delegation-completion.test.ts`:
- Around line 152-155: The helper makeState currently circumvents the branded
run ID contract by casting id as RunbookState['id']; change makeState's id
parameter to accept only rd_-prefixed IDs (e.g., a template/nominal type or a
tighter string union like "rd_*" equivalent to the project's branded
RunbookState['id']) so callers cannot pass arbitrary strings, remove the cast,
and update all fixture callsites in this test file to use rd_... IDs (e.g.,
rd_<whatever>) to ensure tests exercise the branded run identity contract;
reference makeState and RunbookState['id'] when making the change.
In `@packages/cli/__tests__/helpers/test-utils.ts`:
- Around line 506-509: The type guard isRunbookState currently only accepts
runbookRef.source values 'project' | 'plugin' | 'bundled', causing valid
'external' runbook states to be rejected; update the boolean guard in
isRunbookState (the conditional referencing runbookRef.source and
runbookRef.path in test-utils) to also accept 'external' as a valid source while
keeping the existing check for typeof runbookRef.path === 'string', so functions
like readRunbookState will return the runbook state for external sources as
well.
In `@packages/cli/__tests__/services/variable-discovery.test.ts`:
- Around line 503-512: The test only checks rejection for built-in keys via the
input string path; update the test to iterate the same key list (derive it from
BUILTIN_VARIABLES) and assert resolveVariables rejects when the reserved keys
are provided via the other ingress paths (inputJson, inputFile and
environment-style RD_INPUT_*). Specifically, import or reference
BUILTIN_VARIABLES to build the key array, then add cases that call
resolveVariables with { inputJson: JSON.stringify({...}) }, with a temporary
file passed as { inputFile: pathToTempFile } containing the JSON, and with
env-like entries (e.g. simulate RD_INPUT_<KEY>=value) to ensure all parsing
paths throw the /reserved runtime variable/i error for keys like RunId and
RunbookRef.
In `@packages/cli/src/commands/stash.ts`:
- Line 55: The stash command is calling getStepTotal with the logical reference
state.runbook.path which may not be the resolved filesystem path for
external/artifact/delegated runbooks; change the call in the stash handler to
use the resolved, source-aware path (state.runbookPath) when invoking
getStepTotal so it counts steps from the actual file executed (update the
reference in the line that currently passes state.runbook.path to getStepTotal
and ensure any related variables in the stash function use state.runbookPath).
In `@packages/cli/src/helpers/transition-orchestrator.ts`:
- Around line 140-143: The terminal transition helper applyTerminalSideEffects
currently accepts runbookId: string which strips the branded RunId type coming
from RunbookState.id; change the parameter to runbookId: RunId (import RunId)
and update callers to pass the RunbookState.id directly (remove any string
casts) so the branded ID is preserved at the stop/complete boundary; if
releaseRunbook(sessionService, ...) expects string, update its signature to
accept RunId (or add a minimal adapter at the call site) so the branded type
flows through without being unbranded.
In `@packages/cli/src/services/variable-discovery.ts`:
- Around line 232-237: computeWorkPath() currently returns the shared WORK_DIR
which collapses all branches/runs into the same `.rundown/work` and can cause
cross-branch clobbering; change computeWorkPath() to restore isolation by
appending a branch- or run-scoped suffix (e.g., branch name/commit SHA or a
runId) to WORK_DIR so each checkout gets its own workspace, using available
context or a helper that derives a stable per-branch/run token, and ensure the
function (computeWorkPath) remains the single place that computes this path so
callers (who expect a WorkPath) do not need changes; if the shared-path contract
is intentional, instead add tests and documentation and a security note covering
the new global WORK_DIR behavior.
In `@packages/core/__tests__/runbook/compiler.test.ts`:
- Around line 9892-9893: Replace the fake id string cast ('test-run' as
RunbookState['id']) with a real branded RunId value so tests use a valid rd_...
run id; update the persisted fixture by seeding a concrete rd_... string (or
call the repo RunId test helper) for the id property used in these retry-path
tests (the object containing id: ... and runbook: { source: 'project', path:
'parent.md' }) so the snapshots reflect the production rd_ run-id contract;
apply the same change to the other occurrences noted (lines around the second
fixture).
- Line 9912: Add a test variant where childRunbookRef.source is not 'project' to
ensure external/claim refs are preserved: update the test that currently seeds
childRunbookRef: { source: 'project', path: `child-${substepId}.md` } to include
at least one case with a different source (e.g., 'external' or 'claim') and the
same path pattern so the retry logic is exercised; make the same change in the
other test block referenced around lines 10523-10526 to cover both helpers and
verify re-delegation does not normalize external/claim refs back to project
paths.
In `@packages/core/__tests__/runbook/delegation-service-fixtures.ts`:
- Line 97: The fixture uses a cast that bypasses the RunId contract: replace the
literal "run-1" used for the id field (currently typed as RunbookState['id'])
with a contract-valid test RunId (e.g. a string starting with "rd_...") or use
the project's test-ID helper if available; update the id assignment in the
delegation-service fixture so the id value conforms to the RunId format
(referencing the id field typed as RunbookState['id'] in this file) to avoid
masking ID-format regressions.
In `@packages/core/__tests__/runbook/for-iteration-service.test.ts`:
- Around line 54-57: The fixture id cast in makeState (and other test fixtures
like 'test-123' and 'missing-id') bypasses the RunId branded type; replace the
manual casts with calls to assertRunId using a valid rd_ prefixed 32-hex value
(e.g. assertRunId('rd_00000000000000000000000000000000')) so the returned
RunbookState.id is a real RunId; update every occurrence in this test file to
use assertRunId(...) instead of string casts to keep fixtures consistent with
the /^rd_[a-f0-9]{32}$/ contract.
In `@packages/core/__tests__/runbook/types.test.ts`:
- Around line 108-110: The fixture uses an impossible run id by casting 'wf-...'
to RunbookState['id']; update the RunbookState fixture to use a valid branded
run id (prefixed with "rd_") for the id field and remove the cast so the value
conforms to the RunbookState['id'] type; locate the state object in the test
(variable name state, properties id and runbook) and replace the id value with a
real "rd_..." run id string that satisfies the branded type contract.
In `@packages/parser/__tests__/frontmatter.test.ts`:
- Around line 643-654: The test "rejects reserved runtime identity \"%s\" in
inputs" builds a markdown fixture and asserts on diagnostics[0].message using
new RegExp(`${name}.*reserved`, 'i') which can misbehave for variable
interpolation; update the assertions after calling extractFrontmatter(markdown)
to check the message contains the literal name and separately matches a static
/reserved/i (e.g. expect(diagnostics[0].message).toContain(name) and
expect(diagnostics[0].message).toMatch(/reserved/i)) while keeping the rest of
the test (frontmatter?.inputs assertion) unchanged so the test uses literal
substring checks instead of RegExp-from-variable; reference extractFrontmatter,
diagnostics and the test "rejects reserved runtime identity \"%s\" in inputs".
---
Outside diff comments:
In `@packages/claude-code-plugin/__tests__/rdpath-find-integration.test.ts`:
- Around line 393-408: The recovery matcher in
isRecoverableActiveStateLookupError incorrectly looks for 'Session file contains
invalid entries' while RunbookStateManager.loadSession throws 'Session file
contains invalid runbook targeting data'; update
isRecoverableActiveStateLookupError to match the actual thrown message (or use a
regex/contains check that covers both 'invalid entries' and 'invalid runbook
targeting data') so the error is classified as recoverable and the code paths
that silently ignore stale/invalid session state when --dir or RD_WORK_PATH is
provided will execute.
In `@packages/cli/__tests__/commands/delegate.test.ts`:
- Around line 161-167: Add an assertion to this test to verify the persisted
structured runbook identity: after obtaining delegation from ss1 (using
getActiveState, substepStates, ss1, delegation), assert that
delegation.childRunbookRef exists and matches the expected structure (e.g.,
contains the runbook id/ref format your code uses) — this ensures we validate
childRunbookRef is persisted (not just childRunbookPath); update the expectation
alongside the existing childRunId/tokenHash/token checks to assert the exact
shape or required fields of childRunbookRef.
In `@packages/cli/__tests__/helpers/runbook-pipeline.test.ts`:
- Around line 303-317: The tests use makeState(id, ...) to build run state but
pass arbitrary ids like "parent-id" instead of canonical RunId strings; update
makeState to accept or default to a valid RunId (format "rd_<32 hex>") or call
the existing RunId fixture generator so all created states use the rd_<32 hex>
contract; locate makeState in
packages/cli/__tests__/helpers/runbook-pipeline.test.ts and either change its
default id to a canonical RunId or wrap incoming ids with the project’s RunId
factory/util (ensuring tests that intentionally pass invalid ids remain
explicit), then run the suite to verify identity-path tests exercise the real
RunId format.
In `@packages/cli/src/helpers/goto-workflow.ts`:
- Around line 84-91: The function resolveTerminalReleaseModeForRunbook has
loosened the runbookId parameter to plain string, weakening the
RunbookState['id'] (or branded RunId) type guarantee; restore the stronger type
by changing the runbookId parameter back to RunbookState['id'] or the RunId
brand (instead of string) in the function signature and adjust any call sites to
pass the branded id, or if this is intentionally a CLI boundary accept string
but add a short doc comment on resolveTerminalReleaseModeForRunbook stating
callers must validate/parse the RunId format before calling and optionally
perform a runtime validation in the function (use manager.loadSession() and the
existing claim check) to assert the id shape.
In `@packages/cli/src/services/variable-discovery.ts`:
- Around line 34-44: The setExecFileSyncImpl currently replaces the
module-global execFileSyncImpl but does not return a disposer; change
setExecFileSyncImpl(fn: typeof nodeExecFileSync) to capture the current
execFileSyncImpl, assign the new fn to execFileSyncImpl, and return a restore
function that when called reassigns the captured original back to
execFileSyncImpl so tests can deterministically restore the previous
implementation; reference the module-level variable execFileSyncImpl and the
exported function setExecFileSyncImpl when making this change.
In `@packages/core/src/runbook/delegation-service.ts`:
- Around line 679-685: The branch that constructs createResult must reject stale
persisted delegations that lack a canonical childRunbookRef instead of
reconstructing them: in retryDelegation() add a guard that checks
existingDelegation.childRunbookRef is present (and, if applicable, matches any
expected ref) and if it's missing, return/throw a failure indicating
stale/incompatible state rather than calling createDelegation; reference the
retryDelegation function, existingDelegation, childRunbookRef, and the
createDelegation/CreateDelegationResult flow so the code aborts early for stale
delegations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fad75a16-2a2b-4350-a87f-c7decde7d54d
📒 Files selected for processing (97)
.coderabbit.yamlcspell-dictionary.txtdocs/reference/cli.mddocs/reference/runtime.mddocs/spec/cli-output.mddocs/spec/language.mdpackages/claude-code-plugin/__tests__/rdpath-find-integration.test.tspackages/cli/__tests__/commands/claim.test.tspackages/cli/__tests__/commands/delegate.test.tspackages/cli/__tests__/commands/fail.test.tspackages/cli/__tests__/commands/output-format.test.tspackages/cli/__tests__/commands/pass.test.tspackages/cli/__tests__/commands/resolve.test.tspackages/cli/__tests__/commands/run-prompted.test.tspackages/cli/__tests__/commands/run.test.tspackages/cli/__tests__/commands/stash-pop.test.tspackages/cli/__tests__/commands/status.test.tspackages/cli/__tests__/helpers/brand-helpers.tspackages/cli/__tests__/helpers/claim-and-launch.test.tspackages/cli/__tests__/helpers/delegate-inference.test.tspackages/cli/__tests__/helpers/delegation-completion.test.tspackages/cli/__tests__/helpers/execution-emitter.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/__tests__/helpers/normalize-cli-output.test.tspackages/cli/__tests__/helpers/resolve-runbook-plugin-context.test.tspackages/cli/__tests__/helpers/resolve-runbook.test.tspackages/cli/__tests__/helpers/runbook-loader.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.tspackages/cli/__tests__/helpers/scenario-workflow.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/cli/__tests__/helpers/test-utils.tspackages/cli/__tests__/integration/delegate-workflow.test.tspackages/cli/__tests__/integration/scenario-runner.test.tspackages/cli/__tests__/services/execution-helpers.test.tspackages/cli/__tests__/services/execution-loop.test.tspackages/cli/__tests__/services/internal-commands.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/cli/src/commands/delegate.tspackages/cli/src/commands/ls.tspackages/cli/src/commands/prune.tspackages/cli/src/commands/run.tspackages/cli/src/commands/stash.tspackages/cli/src/helpers/execution-emitter.tspackages/cli/src/helpers/goto-workflow.tspackages/cli/src/helpers/resolve-runbook.tspackages/cli/src/helpers/runbook-loader.tspackages/cli/src/helpers/runbook-pipeline.tspackages/cli/src/helpers/transition-orchestrator.tspackages/cli/src/helpers/validate-frontmatter-vars.tspackages/cli/src/services/execution.tspackages/cli/src/services/variable-discovery.tspackages/core/__tests__/helpers/step-factories.tspackages/core/__tests__/runbook/abort-delegation.test.tspackages/core/__tests__/runbook/actor-service.test.tspackages/core/__tests__/runbook/artifact-manifest-toctou.test.tspackages/core/__tests__/runbook/artifact-manifest.test.tspackages/core/__tests__/runbook/artifact-schema.test.tspackages/core/__tests__/runbook/artifact-uri.test.tspackages/core/__tests__/runbook/compiler.test.tspackages/core/__tests__/runbook/create-delegation.test.tspackages/core/__tests__/runbook/delegation-context.test.tspackages/core/__tests__/runbook/delegation-propagation.test.tspackages/core/__tests__/runbook/delegation-scan.test.tspackages/core/__tests__/runbook/delegation-schemas.test.tspackages/core/__tests__/runbook/delegation-service-fixtures.tspackages/core/__tests__/runbook/delegation-service.test.tspackages/core/__tests__/runbook/execution-lifecycle-service.test.tspackages/core/__tests__/runbook/for-iteration-service.test.tspackages/core/__tests__/runbook/retry-delegation.test.tspackages/core/__tests__/runbook/retry-hook.test.tspackages/core/__tests__/runbook/retry-single-substep.test.tspackages/core/__tests__/runbook/runbook-ref.test.tspackages/core/__tests__/runbook/session-reader.test.tspackages/core/__tests__/runbook/session-service.test.tspackages/core/__tests__/runbook/state-schema-version.test.tspackages/core/__tests__/runbook/state.test.tspackages/core/__tests__/runbook/types.test.tspackages/core/__tests__/schemas.test.tspackages/core/src/index.tspackages/core/src/runbook/artifact-errors.tspackages/core/src/runbook/artifact-uri.tspackages/core/src/runbook/claim-id.tspackages/core/src/runbook/delegation-context.tspackages/core/src/runbook/delegation-service.tspackages/core/src/runbook/index.tspackages/core/src/runbook/run-id.tspackages/core/src/runbook/runbook-ref.tspackages/core/src/runbook/session-service.tspackages/core/src/runbook/state.tspackages/core/src/runbook/types.tspackages/core/src/schemas.tspackages/parser/__tests__/frontmatter.test.tspackages/parser/__tests__/outputs-inputs.test.tspackages/parser/src/frontmatter.tspackages/parser/src/index.tspackages/parser/src/parser.tspackages/parser/src/reserved.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/helpers/runbook-pipeline.ts (1)
1014-1023: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winFail closed if
manager.create()returns a different run id.
RunIdis injected into template vars before launch, but every later state/session/event path keys offstate.id. Add an explicit equality check here so the new identity contract cannot silently drift.Suggested guard
const state = await manager.create(prepared.runbookRef, runbook, { runId: prepared.runId, runbookPath, prompted: options.prompted, parentLinkage: options.parentLinkage, runbookSrc: rawContent, templateVars: mergedVariables, frontmatterOutputs: prepared.frontmatter?.outputs ?? [], }); + if (state.id !== prepared.runId) { + throw new Error( + `RunId contract violated: expected ${prepared.runId}, received ${state.id} from RunbookStateManager.create()`, + ); + } stateId = state.id;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/helpers/runbook-pipeline.ts` around lines 1014 - 1023, The code calls manager.create(prepared.runbookRef, runbook, { runId: prepared.runId, ... }) and then assigns stateId = state.id, but doesn't assert the created state's id matches the injected prepared.runId; add an explicit guard after the create call that compares state.id to prepared.runId and fails closed (throw an Error or reject) if they differ, including a clear log/error message that names manager.create, state.id, prepared.runId and the runId mismatch to prevent silent identity drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/cli/src/helpers/runbook-pipeline.ts`:
- Around line 1014-1023: The code calls manager.create(prepared.runbookRef,
runbook, { runId: prepared.runId, ... }) and then assigns stateId = state.id,
but doesn't assert the created state's id matches the injected prepared.runId;
add an explicit guard after the create call that compares state.id to
prepared.runId and fails closed (throw an Error or reject) if they differ,
including a clear log/error message that names manager.create, state.id,
prepared.runId and the runId mismatch to prevent silent identity drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ae0cb033-4fd4-49b5-aa0e-ba9bce9e59db
📒 Files selected for processing (6)
packages/claude-code-plugin/__tests__/rdpath-find-integration.test.tspackages/claude-code-plugin/src/rdpath.tspackages/cli/__tests__/commands/ls.test.tspackages/cli/src/commands/ls.tspackages/cli/src/helpers/context.tspackages/cli/src/helpers/runbook-pipeline.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: scenarios
- GitHub Check: test (24)
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use clear, descriptive variable and function names that convey intent
Add comments for complex logic and non-obvious code sections
Use async/await over callbacks and promise chains
Use const by default, let for variables that need reassignment, avoid var
Use template literals instead of string concatenation
Files:
packages/cli/__tests__/commands/ls.test.tspackages/claude-code-plugin/__tests__/rdpath-find-integration.test.tspackages/claude-code-plugin/src/rdpath.tspackages/cli/src/helpers/context.tspackages/cli/src/commands/ls.tspackages/cli/src/helpers/runbook-pipeline.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety; define explicit types instead of relying on inference
**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript
UseisError(),isNodeError(), orgetErrorMessage()from@rundown-org/core(orpackages/claude-code-plugin/src/shared/errors.tsinside the plugin) — never callError.isError()directly
All exported TypeScript symbols must have TSDoc documentation including description,@paramfor all parameters,@returnsif non-void, and@throwsif exceptions are possible
Use discriminated unions and type narrowing to make invalid states unrepresentable. Guards should express domain conditions through typed return values, never raw action-type string checks.
Use XState's native event system and state graph structure instead of creating artificial state identifiers (like~channelprefixes) for synthetic IDs.
Files:
packages/cli/__tests__/commands/ls.test.tspackages/claude-code-plugin/__tests__/rdpath-find-integration.test.tspackages/claude-code-plugin/src/rdpath.tspackages/cli/src/helpers/context.tspackages/cli/src/commands/ls.tspackages/cli/src/helpers/runbook-pipeline.ts
**/*.test.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Write unit tests with clear arrange-act-assert structure
Files:
packages/cli/__tests__/commands/ls.test.tspackages/claude-code-plugin/__tests__/rdpath-find-integration.test.ts
**/*.{ts,tsx,js,mjs,cjs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,mjs,cjs}: Use camelCase for variable names in TypeScript/JavaScript
Always use async/await for promises in TypeScript/JavaScript
Variable names must match the pattern/^[a-zA-Z_][a-zA-Z0-9_]*$/(alphanumeric and underscore, starting with letter or underscore)
Files:
packages/cli/__tests__/commands/ls.test.tspackages/claude-code-plugin/__tests__/rdpath-find-integration.test.tspackages/claude-code-plugin/src/rdpath.tspackages/cli/src/helpers/context.tspackages/cli/src/commands/ls.tspackages/cli/src/helpers/runbook-pipeline.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Mock injected core services structurally in non-core tests using object-shaped service doubles instead of constructing real core services from mocked modules
Files:
packages/cli/__tests__/commands/ls.test.tspackages/claude-code-plugin/__tests__/rdpath-find-integration.test.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (Custom checks)
**/*.{ts,tsx,js}: Warn if CLI JSON output, command names, or --schema behavior change without corresponding OutputEmitter usage, output schema mapping updates, and schema coverage tests
Pass CLI schema coverage if the PR does not touch CLI output behavior
Pass CLI schema coverage if schema source, CLI exports, and tests are updated together
Warn if files that define persisted runbook state, state snapshots, lifecycle fields, variable storage, delegation state, or schemaVersion behavior change and the PR also silently migrates existing .rundown/runs/ data
Pass persisted state compatibility if the PR does not touch persisted state behavior
Pass persisted state compatibility if the PR preserves compatibility or detects stale state and requires completion, stop, or prune before restart
Warn if policy, sandbox, command execution, file data sources, path resolution, or environment handling changes widen command, path, or environment capabilities without corresponding tests or docs/reference/security.md updates
Pass security policy review if the PR does not touch these areas
Files:
packages/cli/__tests__/commands/ls.test.tspackages/claude-code-plugin/__tests__/rdpath-find-integration.test.tspackages/claude-code-plugin/src/rdpath.tspackages/cli/src/helpers/context.tspackages/cli/src/commands/ls.tspackages/cli/src/helpers/runbook-pipeline.ts
{**/*.test.{ts,tsx,js},docs/reference/security.md}
📄 CodeRabbit inference engine (Custom checks)
Pass security policy review if the PR includes tests or documentation showing traversal, symlink escape, deny precedence, env leakage, shell parsing, and fail-closed behavior were preserved
Files:
packages/cli/__tests__/commands/ls.test.tspackages/claude-code-plugin/__tests__/rdpath-find-integration.test.ts
packages/cli/**/*.ts
⚙️ CodeRabbit configuration file
packages/cli/**/*.ts: CLI package. Focus on:
- OutputEmitter usage for new CLI commands.
- JSON output by default and --text for human-readable output.
- UPPERCASE table headers, 2-space column separators, left-aligned text, and right-aligned numbers.
- Proper error handling, command option validation, and user-facing messages.
- If command JSON output changes, verify schema mappings and tests in packages/cli/src/schemas, packages/core/src/output, and packages/cli/tests.
Files:
packages/cli/__tests__/commands/ls.test.tspackages/cli/src/helpers/context.tspackages/cli/src/commands/ls.tspackages/cli/src/helpers/runbook-pipeline.ts
**/__tests__/**/*.ts
⚙️ CodeRabbit configuration file
**/__tests__/**/*.ts: Test files. Focus on:
- Use Error.isError() instead of instanceof Error except for same-realm custom error classes.
- Mock passthrough pattern: pass through real functions that do not need mocking.
- Test isolation, meaningful assertions, edge cases, and regression coverage for changed behavior.
- Prefer targeted verification commands that match the package touched by the PR.
Files:
packages/cli/__tests__/commands/ls.test.tspackages/claude-code-plugin/__tests__/rdpath-find-integration.test.ts
packages/claude-code-plugin/**/*.ts
⚙️ CodeRabbit configuration file
packages/claude-code-plugin/**/*.ts: Claude Code plugin for runbook orchestration. Focus on:
- MCP integration correctness, runbook discovery, namespace resolution, state lifecycle, and hook behavior.
- Safe path handling for plugin root/data paths and generated files.
- Changes to Zod schemas must stay consistent with checked-in JSON schemas.
Files:
packages/claude-code-plugin/__tests__/rdpath-find-integration.test.tspackages/claude-code-plugin/src/rdpath.ts
packages/cli/src/commands/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
packages/cli/src/commands/**/*.{ts,tsx}: New CLI commands must useOutputEmitterfor consistent output with format-agnostic rendering
UseformatTablefrom../helpers/table-formatter.jsfor direct table formatting in CLI commands when JSON output is not needed
Use uppercase headers with 2-space column separators in CLI table output; left-align text and right-align numbers
Files:
packages/cli/src/commands/ls.ts
🧠 Learnings (18)
📚 Learning: 2026-03-13T07:37:13.038Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 102
File: packages/cli/__tests__/commands/fail.test.ts:134-141
Timestamp: 2026-03-13T07:37:13.038Z
Learning: In the tobyhede/rundown repository, ensure that ErrorResponseSchema in packages/core/src/output/zod-schemas.ts uses command: z.string().optional() (renamed from action) to reflect NoActiveRunbookOutput.action → command across the codebase. Update and verify all references to this field, including related code paths such as OutputEmitter.noActiveRunbook() and json-renderer.ts. During reviews, check that tests (e.g., packages/cli/__tests__/commands/fail.test.ts) align with the new field name and that any expectations or mocks reference command rather than action.
Applied to files:
packages/cli/__tests__/commands/ls.test.tspackages/claude-code-plugin/__tests__/rdpath-find-integration.test.tspackages/claude-code-plugin/src/rdpath.tspackages/cli/src/helpers/context.tspackages/cli/src/commands/ls.tspackages/cli/src/helpers/runbook-pipeline.ts
📚 Learning: 2026-04-06T04:20:01.297Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 177
File: packages/claude-code-plugin/runbooks/planning/write-plan.runbook.md:24-24
Timestamp: 2026-04-06T04:20:01.297Z
Learning: In this repository (tobyhede/rundown), `CLAUDE_PLUGIN_ROOT` is part of a published contract and is guaranteed to already include a trailing `/` (enforced since PR `#174`). Therefore, when reviewing code, do not flag path concatenations that join the variable directly to the next path segment (e.g., `{{ CLAUDE_PLUGIN_ROOT }}schemas/plan.schema.json`) as missing a separator—this resolves correctly because the trailing slash is included in the variable value.
Applied to files:
packages/cli/__tests__/commands/ls.test.tspackages/claude-code-plugin/__tests__/rdpath-find-integration.test.tspackages/claude-code-plugin/src/rdpath.tspackages/cli/src/helpers/context.tspackages/cli/src/commands/ls.tspackages/cli/src/helpers/runbook-pipeline.ts
📚 Learning: 2026-05-03T01:45:03.732Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 255
File: packages/cli/__tests__/commands/status.test.ts:239-249
Timestamp: 2026-05-03T01:45:03.732Z
Learning: In tobyhede/rundown’s status output logic (notably `buildStashedStatus` in `packages/cli`/`packages/core`), when a stashed runbook has `parentLinkage.kind === 'delegation'`, the generated status must omit the `vars` key for delegation-claimed children. Specifically, `rd status` (without `--claim-id`) must not expose inherited variable contents for these delegation-claimed descendants (e.g., ensure output does not contain sensitive values). Only callers using `rd status --claim-id <claimId>` (the capability-holder) are allowed to see the `vars` field for stashed delegated children. This guards against production data leaks; preserve/test this isolation behavior (as fixed in PR `#255`, commit dfba8464).
Applied to files:
packages/cli/__tests__/commands/ls.test.tspackages/cli/src/helpers/context.tspackages/cli/src/commands/ls.tspackages/cli/src/helpers/runbook-pipeline.ts
📚 Learning: 2026-05-03T01:45:37.414Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 255
File: packages/cli/__tests__/commands/stop.test.ts:203-211
Timestamp: 2026-05-03T01:45:37.414Z
Learning: When reviewing code/tests in this repo that handle errors emitted via `OutputEmitter.error(message, code, details)`, do not expect or suggest a `command` field in the emitted error envelope. The error response shape contains only `kind`, `code`, and `error`/message fields (no `command`). If a `command` field is required for error envelopes, it should be implemented by updating `OutputEmitter` itself (as a follow-up), rather than by asserting `response.command` in callers/tests.
Applied to files:
packages/cli/__tests__/commands/ls.test.tspackages/cli/src/helpers/context.tspackages/cli/src/commands/ls.tspackages/cli/src/helpers/runbook-pipeline.ts
📚 Learning: 2026-03-12T12:08:17.780Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 100
File: packages/claude-code-plugin/src/rdpath-core.ts:9-9
Timestamp: 2026-03-12T12:08:17.780Z
Learning: For imports from 'node:path' in the packages/claude-code-plugin package, prefer namespace imports: import * as path from 'node:path'. This is the dominant pattern across 8+ files (e.g., shared/logger.ts, shared/utils.ts, context.ts, gate-loader.ts, gates/plugin-path.ts, shared/config.ts). Do not suggest switching to named imports; preserve the namespace import convention in this package.
Applied to files:
packages/claude-code-plugin/__tests__/rdpath-find-integration.test.tspackages/claude-code-plugin/src/rdpath.ts
📚 Learning: 2026-04-08T00:08:09.549Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 187
File: packages/claude-code-plugin/__tests__/review-schema.test.ts:98-99
Timestamp: 2026-04-08T00:08:09.549Z
Learning: In tobyhede/rundown, enforce the CLAUDE.md `isZodError()` convention for Zod-related errors. Instead of `instanceof ZodError` in both test and production code (and instead of `toThrow(ZodError)` / `expect(...).toBeInstanceOf(ZodError)` in Jest tests), write Jest assertions as: use `expect(fn).toThrow()` (no `ZodError` class argument), then capture the thrown error (or use the matcher’s error value) and assert `expect(isZodError(err)).toBe(true)` using `isZodError` from `packages/claude-code-plugin/src/shared/errors.ts`. Do not accept the `ZodError` class checks even if they might work due to module loading nuances; treat them as violations. Existing occurrences may be considered acknowledged tech debt until cleaned up (e.g., in plan-schema*.test.ts files).
Applied to files:
packages/claude-code-plugin/__tests__/rdpath-find-integration.test.tspackages/claude-code-plugin/src/rdpath.ts
📚 Learning: 2026-05-03T01:44:16.989Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 255
File: packages/cli/src/helpers/goto-workflow.ts:89-90
Timestamp: 2026-05-03T01:44:16.989Z
Learning: In CLI helper code under packages/cli/src/helpers, avoid adding defensive fallbacks like `session.claims ?? {}` when reading `session.claims`. By design, `RunbookStateManager.loadSession()` rejects legacy session payloads and `SessionDataSchema` defines `claims` as a Zod record with `.default({})`, so `session.claims` is always a non-null object once a session is loaded through the manager. If a helper accepts a session from a path that bypasses `RunbookStateManager.loadSession()`, then this guarantee may not hold—ensure the session is loaded through the manager before relying on non-null `claims`.
Applied to files:
packages/cli/src/helpers/context.tspackages/cli/src/helpers/runbook-pipeline.ts
📚 Learning: 2026-04-15T05:28:29.767Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 202
File: packages/cli/__tests__/commands/json-output.test.ts:264-279
Timestamp: 2026-04-15T05:28:29.767Z
Learning: For CLI command implementations in packages/cli/src/commands/**/*.ts, JSON output should be the default and should not require any flag. The --json option must not exist as a no-op or deprecated alias; attempts to use --json should be treated as an unknown/invalid option. Use --text as the opt-in flag for human-readable output, and construct OutputEmitter with the selected mode via { text: options.text } (i.e., pass the boolean derived from the --text flag).
Applied to files:
packages/cli/src/commands/ls.ts
📚 Learning: 2026-04-15T05:28:37.082Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 202
File: CLAUDE.md:22-23
Timestamp: 2026-04-15T05:28:37.082Z
Learning: In tobyhede/rundown (as of PR `#202`), the CLI `--json` flag has been intentionally and fully removed from all commands under `packages/cli/src/commands/**/*.ts`. JSON output is the default for all commands, and `--text` is the opt-in for human-readable output. During review, do not recommend adding `--json` back, do not suggest `--json` as a compatibility alias, and do not report `--json` as a missing flag. Passing `--json` should fail with a non-zero exit code (covered by `packages/cli/__tests__/commands/json-output.test.ts`), so any proposed behavior change should be treated as a deliberate deviation from the breaking change.
Applied to files:
packages/cli/src/commands/ls.ts
📚 Learning: 2026-04-15T05:28:41.182Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 202
File: packages/cli/src/commands/status.ts:22-23
Timestamp: 2026-04-15T05:28:41.182Z
Learning: In the tobyhede/rundown CLI, `--json` was intentionally removed as a flag in PR `#202`: JSON output is the default for all CLI commands (e.g., `status`, `ls`, `pass`, `fail`, `run`, etc.). Do not suggest re-adding `--json` as a deprecated alias or raising it as a backward-compatibility concern in future reviews. If scripts/tests previously used `--json`, update them by removing the flag—they should continue to receive JSON output. This behavior is covered by `packages/cli/__tests__/commands/json-output.test.ts`.
Applied to files:
packages/cli/src/commands/ls.ts
📚 Learning: 2026-04-15T05:28:42.109Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 202
File: packages/cli/src/commands/check.ts:13-15
Timestamp: 2026-04-15T05:28:42.109Z
Learning: For the tobyhede/rundown CLI commands under packages/cli/src/commands/**/*.ts, JSON is the default output format and the --json flag was intentionally removed as a breaking change. Treat any suggestions to re-add --json or to claim its removal is a bug as incorrect. After removing --json, scripts that previously relied on it should receive JSON by default; --text is the opt-in flag for human-readable output. When wiring output, construct OutputEmitter using the text option only (e.g., OutputEmitter with { text: options.text }) and do not use a { json: options.json }-style configuration. Also note that json-output.test.ts asserts that passing --json to commands like status, ls, or pass must fail with a non-zero exit code.
Applied to files:
packages/cli/src/commands/ls.ts
📚 Learning: 2026-04-15T05:28:47.873Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 202
File: packages/cli/src/commands/stash.ts:18-24
Timestamp: 2026-04-15T05:28:47.873Z
Learning: In the tobyhede/rundown repo (post-PR `#202`), all CLI commands in packages/cli/src/commands/**/*.ts should default to JSON output. The --json flag has been intentionally removed (no alias/compat layer): if a user passes --json to any command, the command must exit non-zero. Use --text as the opt-in for human-readable output, and ensure OutputEmitter is constructed with { text: options.text } and withErrorHandling is called with { text: options.text } so behavior is consistent across commands (validated by packages/cli/__tests__/commands/json-output.test.ts).
Applied to files:
packages/cli/src/commands/ls.ts
📚 Learning: 2026-04-15T05:28:47.770Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 202
File: packages/cli/src/commands/pass.ts:27-28
Timestamp: 2026-04-15T05:28:47.770Z
Learning: For the tobyhede/rundown CLI commands under packages/cli/src/commands/**/*.ts, JSON is the default output format. The `--json` flag has been intentionally removed as a breaking change: if a `--json` argument/flag is passed to any command, the command should exit non-zero (covered by packages/cli/__tests__/commands/json-output.test.ts). Do not recommend re-adding `--json` (even as a deprecated alias). Use the `--text` flag to switch to human-readable output, and construct `OutputEmitter` with `{ text: options.text }` (not `{ json: options.json }`).
Applied to files:
packages/cli/src/commands/ls.ts
📚 Learning: 2026-04-15T05:28:47.770Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 202
File: packages/cli/src/commands/pass.ts:27-28
Timestamp: 2026-04-15T05:28:47.770Z
Learning: In packages/cli/src/commands/**/*.ts, follow the CLI output standards: treat JSON as the default output. When `--text` is provided, switch to human-readable output by constructing `OutputEmitter` with `new OutputEmitter({ text: options.text })` and passing `{ text: options.text }` into `withErrorHandling`. Use the `formatTable` helper (from `../helpers/table-formatter.js`) only for command output paths that do not support JSON output.
Applied to files:
packages/cli/src/commands/ls.ts
📚 Learning: 2026-04-15T05:28:46.741Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 202
File: packages/cli/src/commands/claim.ts:28-28
Timestamp: 2026-04-15T05:28:46.741Z
Learning: In the tobyhede/rundown CLI, JSON output is the default for all commands under packages/cli/src/commands/**/*.ts (per PR `#202`). Do not implement or document a deprecated/aliased --json flag—removing or ignoring it is a breaking change: scripts must stop passing --json. Instead, treat --text as the explicit opt-out for human-readable output. Ensure behavior matches the documented/tested contract (see packages/cli/__tests__/commands/json-output.test.ts), including that passing --json to commands such as status/ls/pass results in a non-zero exit code.
Applied to files:
packages/cli/src/commands/ls.ts
📚 Learning: 2026-04-15T05:28:46.741Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 202
File: packages/cli/src/commands/claim.ts:28-28
Timestamp: 2026-04-15T05:28:46.741Z
Learning: In packages/cli/src/commands/**/*.ts, ensure CLI command implementations use `OutputEmitter` with `{ text: options.text }` for output. Treat JSON output as the default behavior (no `--json` flag required). The only supported human-readable override is `--text` (i.e., `{ text: options.text }` when that flag is set). Do not support or pass any `--json` flag—if it appears in command parsing/usage, it should be treated as a hard error by design.
Applied to files:
packages/cli/src/commands/ls.ts
📚 Learning: 2026-04-15T05:28:46.741Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 202
File: packages/cli/src/commands/claim.ts:28-28
Timestamp: 2026-04-15T05:28:46.741Z
Learning: For text-only table formatting commands that do not support JSON output (i.e., commands that only operate in `--text` mode and never emit JSON), use `formatTable` from `../helpers/table-formatter.js` instead of building table output inline or using JSON-capable formatters.
Applied to files:
packages/cli/src/commands/ls.ts
📚 Learning: 2026-03-13T00:57:15.497Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 100
File: packages/cli/src/helpers/runbook-pipeline.ts:707-714
Timestamp: 2026-03-13T00:57:15.497Z
Learning: In packages/cli/src/helpers/runbook-pipeline.ts, use the canonical value for the parent_step from freshDelegation.contextSnapshot.at (produced by deriveExecutionAt() in delegation-service.ts) instead of manually concatenating stepId and substepId. This ensures correct handling for all cases, including multi-level nesting like 1.2.1 in FOR loops. Keep the logic centralized to rely on delegation-service's computation and add tests to cover nested nesting scenarios.
Applied to files:
packages/cli/src/helpers/runbook-pipeline.ts
🔇 Additional comments (9)
packages/cli/src/helpers/context.ts (1)
24-35: Good compatibility bridge for runbook identity resolution.This keeps string-path behavior intact and adds
RunbookRefhandling with clean type narrowing and fail-closed semantics (0when unresolved), which aligns well with persisted-state no-migration expectations.packages/claude-code-plugin/src/rdpath.ts (1)
73-74: Recoverable legacy-session matching update is solid.These added message checks correctly extend the soft-fail path for stale/legacy active-state reads when scope resolution can proceed without hard failure.
packages/claude-code-plugin/__tests__/rdpath-find-integration.test.ts (2)
87-92: RunbookState creation fixture now matches the new identity contract.Using the
{ source, path }descriptor inmanager.create(...)is the right update for the new runbook reference shape.
96-105: Stale-state fixture now correctly uses branded RunId wiring.The
rd_...id is applied consistently acrosssession.jsonand the corresponding runs file, which keeps the stale-state scenario realistic under the new runtime identity model.packages/cli/src/commands/ls.ts (1)
88-94: Good source-aware step resolution with stable display shaping.Using
state.runbookfor step-total resolution while projectingstate.runbook.pathfor the display row is the right split for this command path.packages/cli/__tests__/commands/ls.test.ts (1)
40-71: Strong regression coverage for persisted source identity.This test captures the key shadowed-runbook case and correctly guards against resolving totals from the wrong source.
packages/cli/src/helpers/runbook-pipeline.ts (3)
1470-1480: Nice:parent_stepnow uses the canonical persisted at-position.That keeps
rd claimoutput aligned with the execution snapshot for nested delegation cases instead of reconstructing the path ad hoc.Based on learnings: use the canonical value for the parent_step from freshDelegation.contextSnapshot.at (produced by deriveExecutionAt() in delegation-service.ts) instead of manually concatenating stepId and substepId.
Also applies to: 1520-1530, 1583-1593, 1751-1761
756-789: The sourceRoot contract and CLAUDE_PLUGIN_ROOT derivation are correct; no path widening issue.
resolve-runbook.tsconsistently setssourceRoottopath.join(pluginRoot, 'runbooks')for plugin-sourced runbooks (lines 145, 199, 325). Line 756 ofrunbook-pipeline.tscorrectly derives the plugin root viapath.dirname(sourceRoot), which removes therunbookssuffix and yields the plugin root directory. The contract is aligned and the injection at lines 787–788 is safe. No widening of file/env scope occurs.
1600-1625: childRunbookRef is already validated as a required field at the schema parse level.Stale delegations lacking
childRunbookRefare rejected duringmanager.load()via Zod schema validation (inmakeRunbookStateSchema), not at lines 1601–1604. ThechildRunbookReffield is required inmakeStepDelegationSchemawith no.optional()call, so any persisted delegation missing it will fail the parse at state load time—beforeclaimAndLaunchis ever reached. This early rejection honors the no-migration rule and matches the existing pattern used forschemaVersionchecks.> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/cli/__tests__/helpers/status-builder.test.ts (1)
338-358:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a redaction assertion for delegated stashed status (
varsmust be hidden by default)This delegation-path test validates linkage projection but does not assert that
varsis omitted for non-claim status. Add a regression assertion with non-empty vars to enforce fail-closed behavior.Suggested test hardening
it('surfaces parentLinkage in stashed status', () => { const state = makeState({ + templateVars: brandInitialTemplateVarsForTest({ secret: 'from-template' }), + variables: brandStoredOutputsForTest({ secret: 'from-runtime' }), parentLinkage: { kind: 'delegation', tokenHash: brandDelegationTokenHashForTest( 'sha256:fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210', ), parentRunId: SECOND_PARENT_RUN_ID, parentStepId: '2.1', }, }); const result = buildStashedStatus(state, '/test'); expect(result.parentLinkage).toEqual({ kind: 'delegation', tokenHash: 'sha256:fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210', parentRunId: SECOND_PARENT_RUN_ID, parentStepId: '2.1', }); + expect(result.vars).toBeUndefined(); });Based on learnings: when
parentLinkage.kind === 'delegation',rd statuswithout--claim-idmust omitvarsto prevent inherited variable leakage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/__tests__/helpers/status-builder.test.ts` around lines 338 - 358, The test for buildStashedStatus should be hardened to assert that when state.parentLinkage.kind === 'delegation' the returned stashed status omits sensitive vars: update the makeState call to include a non-empty vars object and add an assertion that result.vars is undefined (or not present) after calling buildStashedStatus('/test'); reference the buildStashedStatus function and the parentLinkage field in the state to enforce the fail-closed behavior for delegation paths (rd status without --claim-id).packages/cli/__tests__/services/execution-loop.test.ts (1)
508-545: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueStale mock:
executeCommandsetup appears unused now that assertion targetsexecuteCommandWithEnv.Line 513 mocks
core.executeCommand, but line 544 asserts oncore.executeCommandWithEnv. Given the production code now usesexecuteCommandWithEnvfor environment injection, this mock is dead code. The test passes becauseexecuteCommandWithEnvis already globally mocked at line 95.Consider removing the unused mock for clarity:
🧹 Remove stale mock
mockManager.load .mockResolvedValueOnce(makeLoopState('1')) .mockResolvedValueOnce(makeLoopState('2')); - jest.mocked(core.executeCommand).mockResolvedValue({ success: true, exitCode: 0 }); - mockActorService.sendAndSync.mockResolvedValue({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/__tests__/services/execution-loop.test.ts` around lines 508 - 545, The test sets a mock for core.executeCommand but the assertion and production code use core.executeCommandWithEnv, so remove the stale mock (the jest.mocked(core.executeCommand).mockResolvedValue(...) call) or replace it with a mock for core.executeCommandWithEnv instead so the mocked behavior matches the asserted call; look for the mock near the top of the test and update it to target executeCommandWithEnv (or simply delete the executeCommand mock) to keep mocks consistent with runExecutionLoop and the final expect.packages/cli/src/services/execution.ts (1)
343-350:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail stale run ids before terminal cleanup, not inside it.
Line 349 adds a late
assertRunId(runbookId)on a boundary that is still typed as plainstring. If a pre-rd_session or stale.rundown/runsentry reaches this path, the loop can emit a terminal event and then throw while releasing session state, which is exactly the kind of silent migration break the PR checklist calls out. Please validate the id at the execution boundary or convert legacy ids into an explicit stale-state/prune flow instead of asserting only during cleanup.As per coding guidelines: "Pass persisted state compatibility if the PR preserves compatibility or detects stale state and requires completion, stop, or prune before restart."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/services/execution.ts` around lines 343 - 350, The code currently calls assertRunId(runbookId) inside applyExecutionTerminalRelease just before sessionService.releaseRunbook, which can throw after emitting terminal events for stale or legacy run ids; instead validate or normalize the run id at the execution boundary (before applyExecutionTerminalRelease is invoked) or implement an explicit stale-id handling path: check run id format using assertRunId (or a new isStaleRunId helper) earlier and either convert legacy ids into a prune/stop flow or short-circuit and mark the session as stale, then call sessionService.releaseRunbook only with a guaranteed valid id; update callers that pass runbookId into applyExecutionTerminalRelease to perform this validation/normalization so assertRunId is not invoked during terminal cleanup.packages/core/__tests__/runbook/retry-delegation.test.ts (1)
24-57: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAssert that retry preserves
childRunbookRef.These cases now pass
childRunbookRefintocreateDelegation, but the suite never checks that the retried delegation still carries it. A regression that keepschildRunbookPathand drops the structured ref would still pass here.Example assertion
expect(result.status).toBe('retried'); if (result.status !== 'retried') return; expect(result.tokenHash).not.toBe(initial.tokenHash); expect(result.token.startsWith(TOKEN_PREFIX)).toBe(true); + expect(result.delegation.childRunbookRef).toEqual({ + source: 'project', + path: 'child.md', + }); const replaced = result.updatedSubstepStates.find((ss) => ss.id === '1');As per coding guidelines: "Test isolation, meaningful assertions, edge cases, and regression coverage for changed behavior."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/__tests__/runbook/retry-delegation.test.ts` around lines 24 - 57, Add an assertion that the retried delegation preserves the structured childRunbookRef: after computing result and locating replaced via result.updatedSubstepStates.find((ss) => ss.id === '1'), assert that replaced?.delegation?.childRunbookRef equals initial.delegation?.childRunbookRef (or equals the original ref object passed into createDelegation). Update the test around createDelegation/retryDelegation to include this check so regressions that drop childRunbookRef in retryDelegation are caught.packages/cli/src/helpers/runbook-pipeline.ts (1)
1672-1681:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAbort the launch when delegated
afterInitcannot claim the child.This callback only sets
invariantViolationand returns.launchRunbook()then keeps going and can enterrunExecutionLoop(), so a child may start running even though the claim/linkage step already failed and the parent was never linked. Make this path throw or otherwise signallaunchRunbook()to stop before the loop starts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/helpers/runbook-pipeline.ts` around lines 1672 - 1681, The afterInit callback currently sets invariantViolation from claimChildForPipeline failure and returns, but launchRunbook still proceeds into runExecutionLoop; change afterInit (or its caller path) to abort the launch immediately when claimResult.ok is false by throwing a specific error or returning a rejected Promise so launchRunbook sees the failure and does not enter runExecutionLoop; ensure the change references the same symbols (afterInit, claimChildForPipeline, invariantViolation, launchRunbook, runExecutionLoop) so the failure short-circuits launchRunbook and surfaces a typed launch-failed envelope instead of allowing execution to continue.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/reference/runtime.md`:
- Around line 318-320: The docs currently mention runbook.source /
RD_RUNBOOK_SOURCE but don't enumerate valid discriminants; update the runtime.md
documentation to explicitly list the allowed RunbookRef.source values (e.g.,
"external", "plugin", "bundled" or whatever the codebase accepts) and add a
short description for each value explaining expected semantics and any required
accompanying fields (for example which values expect URLs, plugin IDs, or
repository-relative paths), and include example JSON/YAML snippets and the
canonical env var name RD_RUNBOOK_SOURCE where applicable so consumers and
schema validators know the exact set of legal values and their meanings; ensure
you reference the RunbookRef.source type name and RD_RUNBOOK_SOURCE token in the
new text so readers can map docs to code/config.
In `@packages/cli/__tests__/helpers/goto-workflow.test.ts`:
- Around line 380-381: Replace the unsafe cast of the runbook id in the test
state with a branded RunId: instead of using the literal cast "'test-id' as
RunbookState['id']", call the test helper brandRunIdForTest('test-id') (or reuse
DEFAULT_RUNBOOK_ID) to produce a RunbookState id that respects the runtime
identity contract; update the test object where id is set (the object with keys
id and runbook) so the id field uses brandRunIdForTest(...) or
DEFAULT_RUNBOOK_ID.
In `@packages/cli/__tests__/helpers/status-builder.test.ts`:
- Around line 15-16: The test is bypassing the RunId branding by casting an
arbitrary string ('test-id' as RunbookState['id']) which can hide format-related
bugs; replace those casts with a real branded fixture using the existing helper
brandRunIdForTest (or reuse PARENT_RUN_ID/SECOND_PARENT_RUN_ID) so the base
fixture uses a valid branded RunId everywhere (look for uses of
RunbookState['id'] in the base fixture and swap the casted 'test-id'/'other-id'
to brandRunIdForTest(...) or the constants PARENT_RUN_ID/SECOND_PARENT_RUN_ID).
In `@packages/cli/__tests__/helpers/test-utils.ts`:
- Around line 502-512: Update the isRunbookState check to reject stale/arbitrary
ids by ensuring state.id is a valid RunId instead of any string: inside
isRunbookState (where state, runbookRef, RUNBOOK_SOURCES, runbookRef.path and
state.runbookPath are checked) add a validation that state.id matches the
canonical RunId format (use the existing isRunId/validateRunId helper if
available, or add a single regex check) and only return true when that RunId
check passes along with the existing runbook.source and path checks.
In `@packages/cli/__tests__/services/variable-discovery.test.ts`:
- Around line 516-551: The tests for reserved runtime identity keys (using
RESERVED_IDENTITY_KEYS and resolveVariables) only use canonical casing and miss
mixed-case variants; update the three test blocks that call resolveVariables for
--input-json, --input-file, and RD_INPUT_* to also exercise mixed-case forms
(e.g., "RunId", "runId", "RUNBOOKREF", etc.) for each name so the parser's
case-insensitive handling is validated: in the --input-json and --input-file
tests pass mixed-case keys in the JSON and YAML inputs respectively, and in the
RD_INPUT_* test set environment variables with mixed-case suffixes (e.g.,
RD_INPUT_RunId) and assert the same rejection/ignore behavior and warning
contents when calling resolveVariables. Ensure you reference
RESERVED_IDENTITY_KEYS and reuse the same assertions and cleanup logic.
In `@packages/cli/src/helpers/runbook-pipeline.ts`:
- Line 756: The code sets pluginRoot using path.dirname(sourceRoot) + path.sep
which yields backslashes on Windows and breaks the CLAUDE_PLUGIN_ROOT
forward-slash contract; change the assignment in runbook-pipeline.ts so that
when source === 'plugin' you use the original sourceRoot (which is guaranteed to
already include a trailing '/') instead of building a path with path.sep—i.e.,
replace the path.dirname(...)+path.sep expression with simply sourceRoot (and
remove the path.sep usage) while keeping the conditional that returns undefined
when source !== 'plugin'.
In `@packages/cli/src/helpers/transition-orchestrator.ts`:
- Line 234: Change the OrchestrateTransitionArgs type so its runbookId field is
typed as RunId (the same type used by RunbookState.id) instead of a plain
string, remove redundant runtime assertions where assertRunId(runbookId) is
called (e.g., in applyTerminalSideEffects and other call sites), and update any
callers to supply a RunId-typed value so validation occurs at the boundary
rather than here; keep assertRunId only if a caller cannot be guaranteed to
provide RunId.
In `@packages/core/src/runbook/session-service.ts`:
- Around line 245-250: The code unconditionally refreshes an existing claim
found by findClaimByChildRunId, which can wrongly return a stale claim from a
different parent/token; change claimRunbook's refresh path to first validate
that the existing claim's linkage (e.g., parentRunId and delegation/token fields
on existing) matches the incoming delegation context, and if it does not, mark
or return the claim as rejected/unlinked instead of refreshing it—do this by
checking the existing object's parent/token fields against the incoming values
before creating refreshed = { ...existing, updatedAt: now }, only call
manager.saveSession(session) and return {status:'claimed', claim:refreshed} when
the linkage matches, otherwise set an appropriate unlinked/rejected status on
session.claims[existing.claimId] and persist that change and return the
unlinked/rejected result.
---
Outside diff comments:
In `@packages/cli/__tests__/helpers/status-builder.test.ts`:
- Around line 338-358: The test for buildStashedStatus should be hardened to
assert that when state.parentLinkage.kind === 'delegation' the returned stashed
status omits sensitive vars: update the makeState call to include a non-empty
vars object and add an assertion that result.vars is undefined (or not present)
after calling buildStashedStatus('/test'); reference the buildStashedStatus
function and the parentLinkage field in the state to enforce the fail-closed
behavior for delegation paths (rd status without --claim-id).
In `@packages/cli/__tests__/services/execution-loop.test.ts`:
- Around line 508-545: The test sets a mock for core.executeCommand but the
assertion and production code use core.executeCommandWithEnv, so remove the
stale mock (the jest.mocked(core.executeCommand).mockResolvedValue(...) call) or
replace it with a mock for core.executeCommandWithEnv instead so the mocked
behavior matches the asserted call; look for the mock near the top of the test
and update it to target executeCommandWithEnv (or simply delete the
executeCommand mock) to keep mocks consistent with runExecutionLoop and the
final expect.
In `@packages/cli/src/helpers/runbook-pipeline.ts`:
- Around line 1672-1681: The afterInit callback currently sets
invariantViolation from claimChildForPipeline failure and returns, but
launchRunbook still proceeds into runExecutionLoop; change afterInit (or its
caller path) to abort the launch immediately when claimResult.ok is false by
throwing a specific error or returning a rejected Promise so launchRunbook sees
the failure and does not enter runExecutionLoop; ensure the change references
the same symbols (afterInit, claimChildForPipeline, invariantViolation,
launchRunbook, runExecutionLoop) so the failure short-circuits launchRunbook and
surfaces a typed launch-failed envelope instead of allowing execution to
continue.
In `@packages/cli/src/services/execution.ts`:
- Around line 343-350: The code currently calls assertRunId(runbookId) inside
applyExecutionTerminalRelease just before sessionService.releaseRunbook, which
can throw after emitting terminal events for stale or legacy run ids; instead
validate or normalize the run id at the execution boundary (before
applyExecutionTerminalRelease is invoked) or implement an explicit stale-id
handling path: check run id format using assertRunId (or a new isStaleRunId
helper) earlier and either convert legacy ids into a prune/stop flow or
short-circuit and mark the session as stale, then call
sessionService.releaseRunbook only with a guaranteed valid id; update callers
that pass runbookId into applyExecutionTerminalRelease to perform this
validation/normalization so assertRunId is not invoked during terminal cleanup.
In `@packages/core/__tests__/runbook/retry-delegation.test.ts`:
- Around line 24-57: Add an assertion that the retried delegation preserves the
structured childRunbookRef: after computing result and locating replaced via
result.updatedSubstepStates.find((ss) => ss.id === '1'), assert that
replaced?.delegation?.childRunbookRef equals initial.delegation?.childRunbookRef
(or equals the original ref object passed into createDelegation). Update the
test around createDelegation/retryDelegation to include this check so
regressions that drop childRunbookRef in retryDelegation are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f72136b2-0494-48f5-9a94-5399a22b187d
📒 Files selected for processing (37)
docs/guides/project-integration.mddocs/reference/runtime.mddocs/reference/security.mddocs/spec/language.mdpackages/cli/__tests__/commands/output-format.test.tspackages/cli/__tests__/helpers/claim-and-launch.test.tspackages/cli/__tests__/helpers/delegate-inference.test.tspackages/cli/__tests__/helpers/delegation-completion.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/test-utils.tspackages/cli/__tests__/services/execution-loop.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/cli/src/commands/abort.tspackages/cli/src/helpers/active-runbook-cleanup.tspackages/cli/src/helpers/runbook-pipeline.tspackages/cli/src/helpers/transition-orchestrator.tspackages/cli/src/services/execution.tspackages/core/__tests__/helpers/effective-vars.tspackages/core/__tests__/runbook/abort-delegation.test.tspackages/core/__tests__/runbook/compiler.test.tspackages/core/__tests__/runbook/create-delegation.test.tspackages/core/__tests__/runbook/delegation-context.test.tspackages/core/__tests__/runbook/delegation-propagation.test.tspackages/core/__tests__/runbook/delegation-service-fixtures.tspackages/core/__tests__/runbook/for-iteration-service.test.tspackages/core/__tests__/runbook/retry-delegation.test.tspackages/core/__tests__/runbook/session-service.test.tspackages/core/__tests__/runbook/types.test.tspackages/core/src/runbook/claim-id.tspackages/core/src/runbook/index.tspackages/core/src/runbook/session-service.tspackages/core/src/runbook/state.tspackages/core/src/runbook/types.tspackages/parser/__tests__/frontmatter.test.ts
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Created PR with unit tests: #268 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/runbook/session-service.ts (1)
410-427: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winComplete the
RunIdcontract onstash().This method still widens the branded id back to
string | null, even though the neighboring session APIs now preserveRunId. That leaves a public type-safety hole in the new identity contract.Suggested fix
- async stash(): Promise<string | null> { + async stash(): Promise<RunId | null> {As per coding guidelines,
**/*.{ts,tsx}: Use TypeScript for type safety; define explicit types instead of relying on inference.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/runbook/session-service.ts` around lines 410 - 427, The stash() method currently returns Promise<string|null> which breaks the RunId branding; change the signature to return Promise<RunId|null>, ensure the stack and activeId are treated as RunId (e.g. defaultStack typed as RunId[] so stack.pop() yields RunId|undefined), and assign session.stashedRunbookId with the RunId typed value before saving via manager.saveSession(session) so the branded type is preserved across stash()/unstash() flows (update any local variables and the method signature to use RunId and handle the undefined->null conversion).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/__tests__/services/variable-discovery.test.ts`:
- Around line 537-553: The test currently only checks result.vars[name] is
undefined which misses canonicalized variants (e.g., RunId/RunbookRef); update
the test in the it.each block that uses RESERVED_IDENTITY_KEY_VARIANTS to assert
there is no case-insensitive key in result.vars (and if available assert not
present in result.providedKeys) by scanning result.vars keys for any key whose
lowercased form equals name.toLowerCase() and failing if found, and do the same
for result.providedKeys to ensure resolveVariables (the function under test)
truly ignores RD_INPUT_* identity keys regardless of casing.
---
Outside diff comments:
In `@packages/core/src/runbook/session-service.ts`:
- Around line 410-427: The stash() method currently returns Promise<string|null>
which breaks the RunId branding; change the signature to return
Promise<RunId|null>, ensure the stack and activeId are treated as RunId (e.g.
defaultStack typed as RunId[] so stack.pop() yields RunId|undefined), and assign
session.stashedRunbookId with the RunId typed value before saving via
manager.saveSession(session) so the branded type is preserved across
stash()/unstash() flows (update any local variables and the method signature to
use RunId and handle the undefined->null conversion).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7fa268dd-56ee-409f-995c-812ee22346b8
⛔ Files ignored due to path filters (1)
packages/cli/__tests__/integration/__snapshots__/scenario-snapshots.test.ts.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (10)
docs/reference/runtime.mdpackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/test-utils.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/cli/src/helpers/runbook-pipeline.tspackages/core/__tests__/runbook/session-service.test.tspackages/core/src/runbook/session-service.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: scenarios
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use clear, descriptive variable and function names that convey intent
Add comments for complex logic and non-obvious code sections
Use async/await over callbacks and promise chains
Use const by default, let for variables that need reassignment, avoid var
Use template literals instead of string concatenation
Files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/test-utils.tspackages/cli/__tests__/helpers/runbook-pipeline.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/core/src/runbook/session-service.tspackages/cli/src/helpers/runbook-pipeline.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety; define explicit types instead of relying on inference
**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript
UseisError(),isNodeError(), orgetErrorMessage()from@rundown-org/core(orpackages/claude-code-plugin/src/shared/errors.tsinside the plugin) — never callError.isError()directly
All exported TypeScript symbols must have TSDoc documentation including description,@paramfor all parameters,@returnsif non-void, and@throwsif exceptions are possible
Use discriminated unions and type narrowing to make invalid states unrepresentable. Guards should express domain conditions through typed return values, never raw action-type string checks.
Use XState's native event system and state graph structure instead of creating artificial state identifiers (like~channelprefixes) for synthetic IDs.
Files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/test-utils.tspackages/cli/__tests__/helpers/runbook-pipeline.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/core/src/runbook/session-service.tspackages/cli/src/helpers/runbook-pipeline.ts
**/*.test.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Write unit tests with clear arrange-act-assert structure
Files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.tspackages/cli/__tests__/helpers/status-builder.test.ts
**/*.{ts,tsx,js,mjs,cjs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,mjs,cjs}: Use camelCase for variable names in TypeScript/JavaScript
Always use async/await for promises in TypeScript/JavaScript
Variable names must match the pattern/^[a-zA-Z_][a-zA-Z0-9_]*$/(alphanumeric and underscore, starting with letter or underscore)
Files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/test-utils.tspackages/cli/__tests__/helpers/runbook-pipeline.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/core/src/runbook/session-service.tspackages/cli/src/helpers/runbook-pipeline.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Mock injected core services structurally in non-core tests using object-shaped service doubles instead of constructing real core services from mocked modules
Files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.tspackages/cli/__tests__/helpers/status-builder.test.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (Custom checks)
**/*.{ts,tsx,js}: Warn if CLI JSON output, command names, or --schema behavior change without corresponding OutputEmitter usage, output schema mapping updates, and schema coverage tests
Pass CLI schema coverage if the PR does not touch CLI output behavior
Pass CLI schema coverage if schema source, CLI exports, and tests are updated together
Warn if files that define persisted runbook state, state snapshots, lifecycle fields, variable storage, delegation state, or schemaVersion behavior change and the PR also silently migrates existing .rundown/runs/ data
Pass persisted state compatibility if the PR does not touch persisted state behavior
Pass persisted state compatibility if the PR preserves compatibility or detects stale state and requires completion, stop, or prune before restart
Warn if policy, sandbox, command execution, file data sources, path resolution, or environment handling changes widen command, path, or environment capabilities without corresponding tests or docs/reference/security.md updates
Pass security policy review if the PR does not touch these areas
Files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/test-utils.tspackages/cli/__tests__/helpers/runbook-pipeline.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/core/src/runbook/session-service.tspackages/cli/src/helpers/runbook-pipeline.ts
{**/*.test.{ts,tsx,js},docs/reference/security.md}
📄 CodeRabbit inference engine (Custom checks)
Pass security policy review if the PR includes tests or documentation showing traversal, symlink escape, deny precedence, env leakage, shell parsing, and fail-closed behavior were preserved
Files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.tspackages/cli/__tests__/helpers/status-builder.test.ts
packages/cli/**/*.ts
⚙️ CodeRabbit configuration file
packages/cli/**/*.ts: CLI package. Focus on:
- OutputEmitter usage for new CLI commands.
- JSON output by default and --text for human-readable output.
- UPPERCASE table headers, 2-space column separators, left-aligned text, and right-aligned numbers.
- Proper error handling, command option validation, and user-facing messages.
- If command JSON output changes, verify schema mappings and tests in packages/cli/src/schemas, packages/core/src/output, and packages/cli/tests.
Files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/cli/__tests__/helpers/test-utils.tspackages/cli/__tests__/helpers/runbook-pipeline.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/cli/src/helpers/runbook-pipeline.ts
**/__tests__/**/*.ts
⚙️ CodeRabbit configuration file
**/__tests__/**/*.ts: Test files. Focus on:
- Use Error.isError() instead of instanceof Error except for same-realm custom error classes.
- Mock passthrough pattern: pass through real functions that do not need mocking.
- Test isolation, meaningful assertions, edge cases, and regression coverage for changed behavior.
- Prefer targeted verification commands that match the package touched by the PR.
Files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/test-utils.tspackages/cli/__tests__/helpers/runbook-pipeline.test.tspackages/cli/__tests__/helpers/status-builder.test.ts
docs/**/*.md
⚙️ CodeRabbit configuration file
docs/**/*.md: Documentation changes. Focus on:
- Accuracy against current CLI behavior and public Rundown specification.
- Updating linked docs together when public syntax, output schema, policy behavior, or runbook discovery changes.
- Keeping examples executable and aligned with package scripts.
Files:
docs/reference/runtime.md
packages/core/src/runbook/**/*.ts
⚙️ CodeRabbit configuration file
packages/core/src/runbook/**/*.ts: This is the XState runbook state machine compiler and runtime. Focus on:
- Type-driven dispatch: types and events should drive logic, not raw string checks.
- No silent mapping: STOP, COMPLETE, BREAK, DEFER, GOTO, NEXT, CONTINUE, and RETRY must preserve their distinct semantics.
- No synthetic IDs: use XState native event system and state graph structure.
- Guard functions must express domain conditions only.
- Persisted state no-migration: this project is pre-release. Do not request
migrations, compatibility shims, fallback reconstruction, or legacy identity
derivation for RunbookState, snapshots, lifecycle, variables, delegation state,
or schemaVersion changes. Flag explicit or implicit migration/adaptation; stale
state should be rejected and require complete, stop, prune, or restart.
Files:
packages/core/src/runbook/session-service.ts
🔇 Additional comments (33)
packages/cli/__tests__/helpers/test-utils.ts (2)
503-513: Good hardening of persisted state validationLine 510 plus Lines 506-512 correctly tighten the helper to accept only canonical
RunIdvalues and known runbook sources. This is a solid no-migration regression gate for test fixtures.
1161-1184: Run ID snapshot normalization is correctly aligned with the new contractLine 1162 now normalizes
rd_<32 lowercase hex>IDs, and the updated rule-order comments (Lines 1177-1183) clearly document collision prevention with the hex catch-all. Good CLI snapshot stability improvement.packages/cli/__tests__/helpers/test-utils.test.ts (1)
388-440: Strong regression coverage for state identity compatibilityLines 389-439 add the right pair of tests: accepting
externalrunbook references while rejecting legacy non-RunId persisted states. This materially strengthens no-migration safety checks.packages/cli/__tests__/helpers/status-builder.test.ts (2)
15-18: RunId branding usage is now correctly enforced in fixturesLines 15-17 and Lines 87-89 correctly move test state construction to branded RunIds and structured
runbookreferences, which closes the old contract-bypass gap.Also applies to: 87-89
295-307: Parent linkage assertions now validate branded run-id propagationThe updated expectations on Lines 295-357 consistently assert branded
parentRunIdvalues across delegation and inline linkage paths. Nice contract-level test tightening.Also applies to: 316-326, 346-357
packages/cli/__tests__/helpers/goto-workflow.test.ts (3)
17-19: Well-structured branded RunId constants.The three RunId constants follow the
rd_<32hex>format and use distinct hex patterns for easy test differentiation. This properly exercises the runtime identity contract.
378-393: makeState now uses branded RunId and object-based runbook shape.This addresses the past review finding. The state builder correctly uses
DEFAULT_RUNBOOK_ID(branded viabrandRunIdForTest) and the newrunbook: { source, path }structure, ensuring tests exercise realistic state identity paths.
495-537: resolveTerminalReleaseModeForRunbook tests correctly use branded RunIds.The delegation claim scenario properly distinguishes parent, child, and claimed run IDs using distinct branded constants. The claim record structure includes all required fields with appropriate types.
packages/cli/src/helpers/runbook-pipeline.ts (7)
105-115: Precise separation of identity-aware template variable types.
PreparedTemplateVariablesexposesRunbookReffor resolver-owned identity, whileRunnableTemplateVariablesaddsRunIdonly when execution identity is minted. This enforces the invariant thatrd resolvenever emits a syntheticRunId.
155-160: deriveClaudePluginRoot addresses Windows path separator contract.The helper normalizes backslashes to forward slashes and strips trailing slashes before deriving the plugin root. This fixes the past review finding about
CLAUDE_PLUGIN_ROOTending with\on Windows.
586-596: Identity validation guards against stale delegation refs.The explicit check that
derivedRunbookRef.source !== runbookRef.source || derivedRunbookRef.path !== runbookRef.pathenforces the no-migration rule by failing closed when persisted delegation identity diverges from the resolved file. This is correct behavior.Note: The path comparison is case-sensitive, which is appropriate for cross-platform consistency—the RunbookRef contract should be case-exact.
726-749: Function overloads ensure type-safe identity propagation.The three-signature pattern (two overloads plus implementation) correctly narrows
PrepareResultvsRunnablePrepareResultbased on whetherPreparedIdentityorRunnableIdentityis passed. Callers get precise return types without unsafe casts.
1017-1030: State creation receives explicit RunbookRef and RunId.
manager.createis called withprepared.runbookReffor canonical identity andprepared.runIdfor execution identity. This clean separation ensures persisted state carries both identity components without conflation.
1607-1642: Delegation resolution uses canonical RunbookRef identity.The switch from file-path discovery to
resolveRunbookRef(cwd, childRunbookRef)ensures delegated children are located by their persisted canonical identity. The distinct error handling forplugin-context-missingvsfile-missingprovides actionable diagnostics.
1328-1353: emitClaimedSuccess centralizes claim output emission.The helper consolidates the repeated pattern of building the payload, emitting status, and returning the success result. Return type
Extract<ClaimResult, { ok: true }>ensures callers get the narrowed success variant.docs/reference/runtime.md (4)
318-320: RunbookRef source values now enumerated.Line 319 explicitly lists the valid discriminants:
project,plugin,bundled, orexternal. This addresses the past review comment about documenting allowed values for therunbook.source/RD_RUNBOOK_SOURCEcontract.
376-379: Reserved variable names updated for new identity model.The reserved name list now includes
runidandrunbookref(case-insensitive), preventing user inputs from colliding with runtime identity variables. This aligns with the language spec updates.
412-436: Built-in variable scoping clarified.The documentation now distinguishes when each identity variable becomes available:
RunbookRef: available during preparation (pre-substitution)RunId: minted only at run start/claimThis matches the code behavior where
rd resolveexposesRunbookRefbut notRunId.
464-466: Delegation inheritance semantics documented.Children explicitly do NOT inherit parent's
RunIdorRunbookRef—they receive fresh values. This is critical for identity isolation and matches the implementation inclaimAndLaunchwhich usesprepareResolvedRunnableRunbookto mint a newRunId.packages/cli/__tests__/helpers/runbook-pipeline.test.ts (9)
43-83: Well-structured test helpers for claim scenarios.
claimRecord,claimedRunbookResult, andmockClaimRunbookSuccessprovide reusable fixtures that correctly model the claim record structure with brandedRunIdandClaimIdtypes. ThemockClaimRunbookSuccessimplementation properly propagates linkage fields.
170-191: Mock delegates to actual buildRunbookRef implementation.Capturing the real
buildRunbookRefand using it as the default mock implementation ensures tests exercise production runbook-ref derivation logic. Individual tests can still override for error/mismatch cases.
537-557: Test verifies PreparedRunbook excludes RunId.The assertion
expect(result.prepared.mergedVariables).not.toHaveProperty('RunId')confirms thatprepareRunbook(non-runnable path) correctly omits execution identity. This enforces the invariant thatrd resolvedoesn't mint synthetic RunIds.
636-660: Test verifies RunnableRunbook includes both identities.
prepareRunnableRunbookproduces aRunnableRunbookwith:
runIdmatching therd_<32hex>patternmergedVariables.RunIdequal torunIdmergedVariables.RunbookRefas the canonical{ source, path }objectThis validates the identity propagation contract.
662-679: Test verifies substitution uses same variable object.The assertion
expect(result.prepared.mergedVariables).toBe(substitutedVars)(reference equality) confirms that the template variables stored inmergedVariablesare the exact same object passed tosubstituteRunbookVariables. This prevents accidental variable drift.
1106-1129: Windows path normalization test validates CLAUDE_PLUGIN_ROOT contract.The test mocks a Windows-style path (
C:\Users\agent\.claude\...) and verifiesCLAUDE_PLUGIN_ROOTis injected with forward slashes (C:/Users/agent/.claude/.../). This exercises thederiveClaudePluginRootfix for the past review finding.
1379-1421: startRunbook test validates identity propagation to state creation.The test constructs a
RunnableRunbookwith explicitrunIdandmergedVariablescontaining bothRunIdandRunbookRef, then verifiesmanager.createreceives:
prepared.runbookRefas first argumentrunId,templateVars.RunId, andtemplateVars.RunbookRefin optionsThis validates the full identity flow from preparation to state persistence.
2375-2379: claimAndLaunch test uses resolveRunbookRef for delegation resolution.The file-missing test now mocks
resolveRunbookRefto return{ ok: false, reason: 'file-missing' }instead of mockingresolveRunbookFile. This aligns with the production code change that resolves delegated children by canonicalRunbookRefidentity.
830-866: Reserved variable validation test updated for new identities.The error message now includes
runid, runbookrefin the reserved variable list, matching the runtime spec update. The test correctly verifies that helper-collision warnings are suppressed when structural validation fails first.packages/cli/__tests__/services/variable-discovery.test.ts (1)
510-535: Good cross-ingress rejection coverage for runtime identity keys.Driving the
--input,--input-json, and--input-filecases fromBUILTIN_VARIABLESis the right shape for this contract and makes future drift much less likely.packages/core/__tests__/runbook/session-service.test.ts (2)
219-241: Good regression coverage for canonical child claim reuse.This test pins the fail-closed behavior nicely: the existing claim stays bound to the original child instead of silently hopping to a new run id.
490-519: Nice coverage for the stashed read/write gate.You verify both sides of the contract here: default claim resolution stays fail-closed for parked children, and
includeStashedonly opens the read-only inspection path.packages/core/src/runbook/session-service.ts (2)
210-279: The claim refresh path is now fail-closed in the right places.Reusing an existing claim only after checking child existence, terminal state, and linkage consistency is the right shape for this persisted-state contract.
341-381: Good atomic cleanup on release.Removing the runbook from default stack, claims, and stash in one locked mutation keeps the session targeting state consistent.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/core/__tests__/runbook/session-service.test.ts (1)
273-291: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding a parallel test for
stoppedlifecycle terminal-child rejection.The current test covers
completedlifecycle. The implementation at lines 245-247 ofsession-service.tstreats bothcompletedandstoppedas terminal. A parallel test forstoppedwould provide symmetric coverage, though both branches share the same code path.Example additional test
it('returns terminal for a stopped claim child', async () => { const parent = await manager.create({ source: 'project', path: 'parent.md' }, mockRunbook, { runbookPath: 'parent.md', }); const linkage = linkageFor(parent.id, 'd2'); const child = await manager.create({ source: 'project', path: 'child.md' }, mockRunbook, { runbookPath: 'child.md', parentLinkage: linkage, }); const claimed = assertClaimed(await sessionService.claimRunbook(child.id, linkage)); await manager.update(child.id, { lifecycle: 'stopped' }); const resolved = await sessionService.getActiveForClaimId(claimed.claim.claimId); expect(resolved.status).toBe('terminal'); if (resolved.status === 'terminal') { expect(resolved.lifecycle).toBe('stopped'); } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/__tests__/runbook/session-service.test.ts` around lines 273 - 291, Add a parallel unit test mirroring the existing "returns terminal for a completed claim child" test but set the child's lifecycle to 'stopped' instead of 'completed'; use the same flow (create parent via manager.create, compute linkage with linkageFor, create child with parentLinkage, call sessionService.claimRunbook and assertClaimed, then await manager.update(child.id, { lifecycle: 'stopped' }) and call sessionService.getActiveForClaimId(claimed.claim.claimId)) and assert resolved.status === 'terminal' and resolved.lifecycle === 'stopped' to ensure the 'stopped' branch in sessionService.getActiveForClaimId is covered.packages/core/src/runbook/session-service.ts (1)
410-428:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winType inconsistency:
stash()returnsstring | nullwhile siblings returnRunId | null.The
stash()method returnsPromise<string | null>butstashRunbook()returnsPromise<RunId | null>,popRunbook()returnsPromise<RunId | null>, andgetStashedRunbookId()returnsPromise<RunId | null>. For type consistency across the RunId migration, this should also returnRunId | null.Proposed fix
- async stash(): Promise<string | null> { + async stash(): Promise<RunId | null> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/runbook/session-service.ts` around lines 410 - 428, The stash() method currently returns Promise<string | null> causing a type mismatch with other APIs; change its signature to Promise<RunId | null> and ensure the local variables/types match RunId (e.g., treat activeId as RunId), updating any stack typing or casts within stash() and the assignment to session.stashedRunbookId so the stored/returned value is a RunId rather than string; keep the existing logic but align types with stashRunbook(), popRunbook(), and getStashedRunbookId() to maintain consistency across the RunId migration.packages/cli/__tests__/helpers/status-builder.test.ts (1)
339-359:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit regression coverage for delegated stashed
varsredaction.Line 339 introduces a delegated stashed case, but the test does not assert
varsis omitted for non-claim callers. Given Line 414 validates stashedvarsinclusion generally, this leaves a security-sensitive gap for delegation paths.Suggested test hardening
describe('parentLinkage projection', () => { @@ it('surfaces parentLinkage in stashed status', () => { const state = makeState({ + templateVars: brandInitialTemplateVarsForTest({ secret: 'top-secret' }), + variables: brandStoredOutputsForTest({ PlanPath: '/work/plan.json' }), parentLinkage: { kind: 'delegation', tokenHash: brandDelegationTokenHashForTest( 'sha256:fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210', ), parentRunId: SECOND_PARENT_RUN_ID, parentStepId: '2.1', }, }); const result = buildStashedStatus(state, '/test'); expect(result.parentLinkage).toEqual({ kind: 'delegation', tokenHash: 'sha256:fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210', parentRunId: SECOND_PARENT_RUN_ID, parentStepId: '2.1', }); + expect(result.vars).toBeUndefined(); }); });Based on learnings, delegation-claimed stashed descendants must not expose
varswithout--claim-idto prevent sensitive data leakage.Also applies to: 414-424
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/__tests__/helpers/status-builder.test.ts` around lines 339 - 359, The test adds a delegated parentLinkage case but must also assert that stashed vars are redacted for non-claim callers; in the existing test that calls makeState({... parentLinkage ...}) and buildStashedStatus(state, '/test'), add an assertion that result.vars is undefined (or not present) to ensure delegated stashed descendants do not expose vars without a --claim-id, while keeping the existing parentLinkage equality assertion intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/reference/runtime.md`:
- Line 319: Update the runbook identity docs to broaden the contract for
runbook.path: change the current "source-root-relative Markdown path" wording to
specify per-source semantics—for source values project, plugin, or bundled,
runbook.path is a safe source-root-relative Markdown path; for source value
external, runbook.path may be an external artifact URI or reference (e.g., an
http(s)/gs:///s3:// URI or other runtime artifact identifier) to match the
runtime identity and artifact URI handling introduced in the PR; ensure the
description still lists allowed source values (`project`, `plugin`, `bundled`,
`external`) and clarifies that consumers should interpret path according to
source-kind.
In `@packages/cli/__tests__/helpers/runbook-pipeline.test.ts`:
- Around line 1452-1467: Replace the string-literal RunId values in the prepared
test fixture with the branded helper: use brandRunIdForTest(...) for both
prepared.runId and prepared.mergedVariables.RunId instead of
`rd_${'c'.repeat(32)}` so the object matches the typed RunnableRunbook and keeps
consistency with the other test data (see other usage around
makeStep()/brandRunIdForTest). Ensure both places use the same brandRunIdForTest
call so types and test invariants remain consistent.
In `@packages/cli/__tests__/helpers/test-utils.ts`:
- Around line 503-512: The helper accepts any file named `${id}.json` even when
the embedded state.id doesn't match the requested id; update readRunbookState to
verify the requested id (the function parameter, e.g., id) is canonical and
strictly equals the parsed state.id before returning a value—if they differ,
return null. Concretely, after confirming isRunId(state.id) and hasKnownSource,
add a check that state.id === id (or parsed.id === id) and reject (return null)
when mismatched; apply the same comparison in the other corresponding branch
(the block around lines 526-533) so filename and embedded run id must match.
In `@packages/cli/src/helpers/runbook-pipeline.ts`:
- Around line 1477-1487: The success envelopes are passing the outer stepId
instead of the actual delegated node ID, violating the ClaimResult.stepId
contract; update every call to emitClaimedSuccess in this function (e.g., the
call shown and the other calls around emitClaimedSuccess at the other branches)
to pass substepId ?? stepId for the stepId parameter so the ClaimResult.stepId
contains the substep when present; keep all other fields the same and apply the
same change to the other emitClaimedSuccess invocations in this function.
- Around line 1607-1642: The code assumes freshDelegation.childRunbookRef is
always defined before calling resolveRunbookRef; add an explicit check on
freshDelegation.childRunbookRef (or the local childRunbookRef) immediately
before resolveRunbookRef and fail with a clear "stale delegation" prepare-failed
response if it's undefined — e.g., return { ok:false, reason:'prepare-failed',
runbook: childDisplayPath, code:'STALE_DELEGATION', cause: 'Delegation refers to
missing childRunbookRef; resume/complete/prune the parent run before retrying',
details:{ runbook: childDisplayPath } } — then only call resolveRunbookRef and
later prepareResolvedRunnableRunbook when childRunbookRef is present. Ensure you
reference freshDelegation.childRunbookRef, childRunbookRef, resolveRunbookRef,
and prepareResolvedRunnableRunbook in the change.
---
Outside diff comments:
In `@packages/cli/__tests__/helpers/status-builder.test.ts`:
- Around line 339-359: The test adds a delegated parentLinkage case but must
also assert that stashed vars are redacted for non-claim callers; in the
existing test that calls makeState({... parentLinkage ...}) and
buildStashedStatus(state, '/test'), add an assertion that result.vars is
undefined (or not present) to ensure delegated stashed descendants do not expose
vars without a --claim-id, while keeping the existing parentLinkage equality
assertion intact.
In `@packages/core/__tests__/runbook/session-service.test.ts`:
- Around line 273-291: Add a parallel unit test mirroring the existing "returns
terminal for a completed claim child" test but set the child's lifecycle to
'stopped' instead of 'completed'; use the same flow (create parent via
manager.create, compute linkage with linkageFor, create child with
parentLinkage, call sessionService.claimRunbook and assertClaimed, then await
manager.update(child.id, { lifecycle: 'stopped' }) and call
sessionService.getActiveForClaimId(claimed.claim.claimId)) and assert
resolved.status === 'terminal' and resolved.lifecycle === 'stopped' to ensure
the 'stopped' branch in sessionService.getActiveForClaimId is covered.
In `@packages/core/src/runbook/session-service.ts`:
- Around line 410-428: The stash() method currently returns Promise<string |
null> causing a type mismatch with other APIs; change its signature to
Promise<RunId | null> and ensure the local variables/types match RunId (e.g.,
treat activeId as RunId), updating any stack typing or casts within stash() and
the assignment to session.stashedRunbookId so the stored/returned value is a
RunId rather than string; keep the existing logic but align types with
stashRunbook(), popRunbook(), and getStashedRunbookId() to maintain consistency
across the RunId migration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c8ed6236-42a7-47ad-a124-b6318c98faf9
⛔ Files ignored due to path filters (1)
packages/cli/__tests__/integration/__snapshots__/scenario-snapshots.test.ts.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (10)
docs/reference/runtime.mdpackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/test-utils.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/cli/src/helpers/runbook-pipeline.tspackages/core/__tests__/runbook/session-service.test.tspackages/core/src/runbook/session-service.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use clear, descriptive variable and function names that convey intent
Add comments for complex logic and non-obvious code sections
Use async/await over callbacks and promise chains
Use const by default, let for variables that need reassignment, avoid var
Use template literals instead of string concatenation
Files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/src/helpers/runbook-pipeline.tspackages/core/src/runbook/session-service.tspackages/cli/__tests__/helpers/test-utils.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety; define explicit types instead of relying on inference
**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript
UseisError(),isNodeError(), orgetErrorMessage()from@rundown-org/core(orpackages/claude-code-plugin/src/shared/errors.tsinside the plugin) — never callError.isError()directly
All exported TypeScript symbols must have TSDoc documentation including description,@paramfor all parameters,@returnsif non-void, and@throwsif exceptions are possible
Use discriminated unions and type narrowing to make invalid states unrepresentable. Guards should express domain conditions through typed return values, never raw action-type string checks.
Use XState's native event system and state graph structure instead of creating artificial state identifiers (like~channelprefixes) for synthetic IDs.
Files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/src/helpers/runbook-pipeline.tspackages/core/src/runbook/session-service.tspackages/cli/__tests__/helpers/test-utils.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.ts
**/*.test.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Write unit tests with clear arrange-act-assert structure
Files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.ts
**/*.{ts,tsx,js,mjs,cjs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,mjs,cjs}: Use camelCase for variable names in TypeScript/JavaScript
Always use async/await for promises in TypeScript/JavaScript
Variable names must match the pattern/^[a-zA-Z_][a-zA-Z0-9_]*$/(alphanumeric and underscore, starting with letter or underscore)
Files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/src/helpers/runbook-pipeline.tspackages/core/src/runbook/session-service.tspackages/cli/__tests__/helpers/test-utils.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Mock injected core services structurally in non-core tests using object-shaped service doubles instead of constructing real core services from mocked modules
Files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (Custom checks)
**/*.{ts,tsx,js}: Warn if CLI JSON output, command names, or --schema behavior change without corresponding OutputEmitter usage, output schema mapping updates, and schema coverage tests
Pass CLI schema coverage if the PR does not touch CLI output behavior
Pass CLI schema coverage if schema source, CLI exports, and tests are updated together
Warn if files that define persisted runbook state, state snapshots, lifecycle fields, variable storage, delegation state, or schemaVersion behavior change and the PR also silently migrates existing .rundown/runs/ data
Pass persisted state compatibility if the PR does not touch persisted state behavior
Pass persisted state compatibility if the PR preserves compatibility or detects stale state and requires completion, stop, or prune before restart
Warn if policy, sandbox, command execution, file data sources, path resolution, or environment handling changes widen command, path, or environment capabilities without corresponding tests or docs/reference/security.md updates
Pass security policy review if the PR does not touch these areas
Files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/src/helpers/runbook-pipeline.tspackages/core/src/runbook/session-service.tspackages/cli/__tests__/helpers/test-utils.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.ts
{**/*.test.{ts,tsx,js},docs/reference/security.md}
📄 CodeRabbit inference engine (Custom checks)
Pass security policy review if the PR includes tests or documentation showing traversal, symlink escape, deny precedence, env leakage, shell parsing, and fail-closed behavior were preserved
Files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.ts
packages/cli/**/*.ts
⚙️ CodeRabbit configuration file
packages/cli/**/*.ts: CLI package. Focus on:
- OutputEmitter usage for new CLI commands.
- JSON output by default and --text for human-readable output.
- UPPERCASE table headers, 2-space column separators, left-aligned text, and right-aligned numbers.
- Proper error handling, command option validation, and user-facing messages.
- If command JSON output changes, verify schema mappings and tests in packages/cli/src/schemas, packages/core/src/output, and packages/cli/tests.
Files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/src/helpers/runbook-pipeline.tspackages/cli/__tests__/helpers/test-utils.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.ts
**/__tests__/**/*.ts
⚙️ CodeRabbit configuration file
**/__tests__/**/*.ts: Test files. Focus on:
- Use Error.isError() instead of instanceof Error except for same-realm custom error classes.
- Mock passthrough pattern: pass through real functions that do not need mocking.
- Test isolation, meaningful assertions, edge cases, and regression coverage for changed behavior.
- Prefer targeted verification commands that match the package touched by the PR.
Files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/__tests__/helpers/test-utils.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.ts
packages/core/src/runbook/**/*.ts
⚙️ CodeRabbit configuration file
packages/core/src/runbook/**/*.ts: This is the XState runbook state machine compiler and runtime. Focus on:
- Type-driven dispatch: types and events should drive logic, not raw string checks.
- No silent mapping: STOP, COMPLETE, BREAK, DEFER, GOTO, NEXT, CONTINUE, and RETRY must preserve their distinct semantics.
- No synthetic IDs: use XState native event system and state graph structure.
- Guard functions must express domain conditions only.
- Persisted state no-migration: this project is pre-release. Do not request
migrations, compatibility shims, fallback reconstruction, or legacy identity
derivation for RunbookState, snapshots, lifecycle, variables, delegation state,
or schemaVersion changes. Flag explicit or implicit migration/adaptation; stale
state should be rejected and require complete, stop, prune, or restart.
Files:
packages/core/src/runbook/session-service.ts
docs/**/*.md
⚙️ CodeRabbit configuration file
docs/**/*.md: Documentation changes. Focus on:
- Accuracy against current CLI behavior and public Rundown specification.
- Updating linked docs together when public syntax, output schema, policy behavior, or runbook discovery changes.
- Keeping examples executable and aligned with package scripts.
Files:
docs/reference/runtime.md
🧠 Learnings (11)
📚 Learning: 2026-03-13T07:37:13.038Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 102
File: packages/cli/__tests__/commands/fail.test.ts:134-141
Timestamp: 2026-03-13T07:37:13.038Z
Learning: In the tobyhede/rundown repository, ensure that ErrorResponseSchema in packages/core/src/output/zod-schemas.ts uses command: z.string().optional() (renamed from action) to reflect NoActiveRunbookOutput.action → command across the codebase. Update and verify all references to this field, including related code paths such as OutputEmitter.noActiveRunbook() and json-renderer.ts. During reviews, check that tests (e.g., packages/cli/__tests__/commands/fail.test.ts) align with the new field name and that any expectations or mocks reference command rather than action.
Applied to files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/src/helpers/runbook-pipeline.tspackages/core/src/runbook/session-service.tspackages/cli/__tests__/helpers/test-utils.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.ts
📚 Learning: 2026-04-06T04:20:01.297Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 177
File: packages/claude-code-plugin/runbooks/planning/write-plan.runbook.md:24-24
Timestamp: 2026-04-06T04:20:01.297Z
Learning: In this repository (tobyhede/rundown), `CLAUDE_PLUGIN_ROOT` is part of a published contract and is guaranteed to already include a trailing `/` (enforced since PR `#174`). Therefore, when reviewing code, do not flag path concatenations that join the variable directly to the next path segment (e.g., `{{ CLAUDE_PLUGIN_ROOT }}schemas/plan.schema.json`) as missing a separator—this resolves correctly because the trailing slash is included in the variable value.
Applied to files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/src/helpers/runbook-pipeline.tspackages/core/src/runbook/session-service.tspackages/cli/__tests__/helpers/test-utils.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.ts
📚 Learning: 2026-05-03T01:45:03.732Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 255
File: packages/cli/__tests__/commands/status.test.ts:239-249
Timestamp: 2026-05-03T01:45:03.732Z
Learning: In tobyhede/rundown’s status output logic (notably `buildStashedStatus` in `packages/cli`/`packages/core`), when a stashed runbook has `parentLinkage.kind === 'delegation'`, the generated status must omit the `vars` key for delegation-claimed children. Specifically, `rd status` (without `--claim-id`) must not expose inherited variable contents for these delegation-claimed descendants (e.g., ensure output does not contain sensitive values). Only callers using `rd status --claim-id <claimId>` (the capability-holder) are allowed to see the `vars` field for stashed delegated children. This guards against production data leaks; preserve/test this isolation behavior (as fixed in PR `#255`, commit dfba8464).
Applied to files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/src/helpers/runbook-pipeline.tspackages/core/src/runbook/session-service.tspackages/cli/__tests__/helpers/test-utils.tspackages/core/__tests__/runbook/session-service.test.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.ts
📚 Learning: 2026-05-03T01:45:37.414Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 255
File: packages/cli/__tests__/commands/stop.test.ts:203-211
Timestamp: 2026-05-03T01:45:37.414Z
Learning: When reviewing code/tests in this repo that handle errors emitted via `OutputEmitter.error(message, code, details)`, do not expect or suggest a `command` field in the emitted error envelope. The error response shape contains only `kind`, `code`, and `error`/message fields (no `command`). If a `command` field is required for error envelopes, it should be implemented by updating `OutputEmitter` itself (as a follow-up), rather than by asserting `response.command` in callers/tests.
Applied to files:
packages/cli/__tests__/helpers/test-utils.test.tspackages/cli/__tests__/helpers/goto-workflow.test.tspackages/cli/src/helpers/runbook-pipeline.tspackages/cli/__tests__/helpers/test-utils.tspackages/cli/__tests__/helpers/status-builder.test.tspackages/cli/__tests__/services/variable-discovery.test.tspackages/cli/__tests__/helpers/runbook-pipeline.test.ts
📚 Learning: 2026-03-13T00:57:15.497Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 100
File: packages/cli/src/helpers/runbook-pipeline.ts:707-714
Timestamp: 2026-03-13T00:57:15.497Z
Learning: In packages/cli/src/helpers/runbook-pipeline.ts, use the canonical value for the parent_step from freshDelegation.contextSnapshot.at (produced by deriveExecutionAt() in delegation-service.ts) instead of manually concatenating stepId and substepId. This ensures correct handling for all cases, including multi-level nesting like 1.2.1 in FOR loops. Keep the logic centralized to rely on delegation-service's computation and add tests to cover nested nesting scenarios.
Applied to files:
packages/cli/src/helpers/runbook-pipeline.ts
📚 Learning: 2026-05-03T01:44:16.989Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 255
File: packages/cli/src/helpers/goto-workflow.ts:89-90
Timestamp: 2026-05-03T01:44:16.989Z
Learning: In CLI helper code under packages/cli/src/helpers, avoid adding defensive fallbacks like `session.claims ?? {}` when reading `session.claims`. By design, `RunbookStateManager.loadSession()` rejects legacy session payloads and `SessionDataSchema` defines `claims` as a Zod record with `.default({})`, so `session.claims` is always a non-null object once a session is loaded through the manager. If a helper accepts a session from a path that bypasses `RunbookStateManager.loadSession()`, then this guarantee may not hold—ensure the session is loaded through the manager before relying on non-null `claims`.
Applied to files:
packages/cli/src/helpers/runbook-pipeline.ts
📚 Learning: 2026-02-26T06:27:17.519Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 51
File: packages/core/src/schemas.ts:438-487
Timestamp: 2026-02-26T06:27:17.519Z
Learning: In the rundown codebase, when evolving persisted state schemas in the core package, do not perform automatic migrations. Instead, implement a roll-forward approach: detect stale or incompatible state and prompt the user to take action (e.g., restart the session with a fresh state). This guideline is documented in CLAUDE.md. During code reviews of TypeScript files under packages/core/src, ensure changes are staged as non-destructive, include clear user prompts or upgrade paths, and avoid migrations that modify existing persisted data automatically.
Applied to files:
packages/core/src/runbook/session-service.ts
📚 Learning: 2026-04-24T01:48:14.441Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 227
File: packages/core/src/runbook/retry-hook.ts:43-57
Timestamp: 2026-04-24T01:48:14.441Z
Learning: In tobyhede/rundown, `asTemplateVars` should intentionally drop `boolean` and `null` template variable values at the `retry-hook` boundary (after upstream stringification). This behavior is consistent with `TemplateVarValue` / `TemplateVarValueSchema` and the upstream resolution contract, so do NOT treat this filtering as data loss or a type-contract violation in future code reviews—if `boolean`/`null` are present here, they should be excluded with a warning rather than passed through.
Applied to files:
packages/core/src/runbook/session-service.ts
📚 Learning: 2026-04-24T01:48:48.712Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 227
File: packages/core/__tests__/runbook/compiler.test.ts:9985-10000
Timestamp: 2026-04-24T01:48:48.712Z
Learning: In `packages/core/__tests__/runbook/**/*.test.ts`, avoid using `as any` when preparing hydrated/persisted snapshots for `createActor(machine, { snapshot })`. Instead, construct the persisted snapshot in a typed way (e.g., start from `baseSnap` and build the final snapshot via object spread so TypeScript can infer `typeof baseSnap`). This keeps the persisted snapshot/context shape checked at compile time and prevents schema drift from bypassing the type system.
Applied to files:
packages/core/__tests__/runbook/session-service.test.ts
📚 Learning: 2026-03-15T08:52:14.181Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 109
File: docs/FORMAT.md:19-19
Timestamp: 2026-03-15T08:52:14.181Z
Learning: Document that the ws lexical rule is a general definition (space or tab) while individual productions may restrict to spaces only. When reviewing docs and related code, ensure this nuance is clearly stated (e.g., in docs/FORMAT.md and related code comments), and reference the relevant code paths and tests (e.g., parser helpers and tests around PASS usage and tab handling). Prefer explicit notes over implying universal tab support, and consider adding automated checks or CI validation to enforce consistency between the lexical rule and production-specific behavior.
Applied to files:
docs/reference/runtime.md
📚 Learning: 2026-03-31T01:30:36.008Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 166
File: packages/claude-code-plugin/skills/writing-runbooks/SKILL.md:140-140
Timestamp: 2026-03-31T01:30:36.008Z
Learning: In this repository, when writing Handlebars templates in Markdown, spaced and unspaced variable forms are both valid. Do not treat `{{ item }}` (with whitespace around the variable name) as a style violation; it is intentionally supported and is used in documentation for readability. Only flag Handlebars syntax issues if they are syntactically invalid beyond whitespace (e.g., malformed braces or invalid expression).
Applied to files:
docs/reference/runtime.md
🔇 Additional comments (25)
packages/core/src/runbook/session-service.ts (2)
60-70: LGTM! Clean conversion helper with proper optional field spreading.The
claimRecordToDelegationLinkagehelper correctly reconstructs aDelegationLinkagefrom aClaimRecord, preserving optional fields only when present. This supports the linkage validation in the refreshedclaimRunbookflow.
210-280: Robust delegation-first claim flow with fail-closed linkage validation.The restructured
claimRunbooknow:
- Checks for an existing claim by delegation linkage first (lines 214-239)
- Validates child state existence and lifecycle before refresh
- Validates persisted linkage against incoming linkage before any mutation
- Falls through to childRunId-based lookup with the same validation
This addresses the prior concern about refreshing stale claims from different parent/token linkages. The fail-closed behavior correctly rejects mismatches rather than silently propagating state corruption.
packages/core/__tests__/runbook/session-service.test.ts (4)
9-11: LGTM! Proper RunId type imports and test helper usage.Clean import of
RunIdtype andbrandRunIdForTesthelper for constructing synthetic RunIds in test scenarios.
36-38: LGTM! Tests updated to use{ source, path }runbook reference shape.All
manager.create()calls now use the structured{ source: 'project', path: '...' }form, aligning with the broader RunbookRef migration.Also applies to: 46-51, 64-72
219-241: Good coverage: Delegation-first claim lookup with synthetic missing child.This test validates the critical invariant that an existing delegation claim is refreshed and returned even when the caller provides a different (nonexistent)
childRunId. ThebrandRunIdForTestusage correctly creates a well-formed but nonexistent RunId to exercise this path.
397-424: Solid regression test for linkage-mismatch on existing child claim refresh.This test ensures that when a child's persisted
parentLinkageis updated to differ from an existing claim record, subsequentclaimRunbookcalls correctly returnlinkage-mismatchrather than silently refreshing the stale claim. The assertion at lines 417-422 verifies thepersistedfield contains the original claim's linkage (converted viaclaimRecordToDelegationLinkage).packages/cli/__tests__/helpers/status-builder.test.ts (1)
15-18: RunId/RunbookRef fixture updates look correct.The branded
RunIdconstants and{ source, path }runbook shape are consistently applied and improve contract fidelity in these tests.Also applies to: 87-88, 295-307, 316-326, 346-357
packages/cli/__tests__/services/variable-discovery.test.ts (6)
37-42: Well-structured identity key test matrix.Deriving canonical names from
BUILTIN_VARIABLESties the test to the implementation's source of truth, preventing drift. The mixed-case variants ensure case-insensitive rejection is verified across input paths.
183-190: Contract clearly expressed: registered ≠ emitted.The test explicitly verifies that
BUILTIN_VARIABLEScontains the identity keys whilegetBuiltinVariables()does not emit them. This separation is the core of the runtime identity model—the keys are reserved for later injection by the runtime, not discovery.
151-157: WorkPath behavioral change correctly captured.The test accurately reflects the new contract:
WorkPathis now fixed toWORK_DIRregardless of git context, whileBranchremains independently derived. This simplifies path handling and avoids issues with branch-name sanitization.
510-516: Prior review item addressed: parameterized identity key rejection for--input.The
it.eachpattern withRESERVED_IDENTITY_KEY_VARIANTSensures all casing variants are tested for the--inputflag path.
518-535: Prior review item addressed:--input-jsonand--input-fileingress paths tested.Both paths now have parameterized coverage for identity key rejection. The
inputJsonformat (${name}="shadow") correctly passes a JSON string value, and theinputFiletests create isolated temp files per variant.
537-553: Prior review item addressed:RD_INPUT_*env bridge tested with correct warn-not-throw semantics.The env bridge path correctly expects a warning rather than an exception—environment variables are ambient inputs, so the CLI warns and ignores rather than failing hard. The assertion verifies both the env key name and "reserved" appear in the warning, ensuring the user understands why the variable was dropped. Test isolation is maintained via
try/finally.packages/cli/__tests__/helpers/runbook-pipeline.test.ts (12)
1-34: Type imports for identity-aware testing look correct.The new imports (
ClaimId,ClaimRecord,ClaimRunbookResult,DelegationLinkage,RunId,PreparedRunbook,RunPipelineContext,RunnableRunbook) align with the PR's goal of making RunbookRef and RunId first-class in the runtime contract. ThebrandRunIdForTesthelper import ensures branded types are used correctly in test data.
50-83: Well-structured test helpers for delegation claim flows.The
claimRecord,claimedRunbookResult, andmockClaimRunbookSuccesshelpers centralize claim-related test data, reducing duplication and ensuring consistent shapes across tests. ThemockClaimRunbookSuccessimplementation correctly passes through linkage fields, which is essential for testing the delegation identity contract.
232-243: BUILTIN_VARIABLES mock correctly includes identity variables.Adding
RunIdandRunbookRefto the mock reflects the production change where these are now reserved runtime variables. This ensures tests accurately simulate the variable discovery behavior.
181-191: Mock passthrough pattern for resolve-runbook is correctly applied.Spreading
actualResolveRunbookand defaultingbuildRunbookRefto the real implementation ensures tests exercise actual ref-building logic while allowing selective overrides for error/mismatch scenarios. This follows the mock passthrough pattern from the coding guidelines.
537-557: Critical test for RunbookRef-without-RunId contract.This test correctly validates that
prepareRunbookincludesRunbookRefin merged variables but does not includeRunId. This distinction is essential—onlyprepareRunnableRunbookshould generate and attach a RunId. Theexpect(...).not.toHaveProperty('RunId')assertion is the right check.
615-634: Identity mismatch detection is fail-fast.This test validates a security-relevant behavior: when resolved identity differs from request identity, the pipeline fails immediately without reading file contents. The assertion
expect(fsPromises.readFile).not.toHaveBeenCalled()confirms fail-closed behavior before I/O.
636-660: RunId propagation in runnable runbooks is correctly tested.The test validates that
prepareRunnableRunbookgenerates a RunId, includes it in bothresult.prepared.runIdandresult.prepared.mergedVariables.RunId, and that they match. The type annotationsconst runnableRunId: RunIdandconst templateRunId: RunIdenforce the branded type contract at the type level.
1106-1129: Windows path normalization for CLAUDE_PLUGIN_ROOT is tested.This test confirms that backslash Windows paths are normalized to forward slashes in
CLAUDE_PLUGIN_ROOT. This is critical for template variable consistency—runbook authors shouldn't need to handle platform-specific path separators. The explicitString.rawusage for the input path is a good pattern for Windows path literals.
1379-1421: startRunbook identity propagation is correctly verified.The test validates that
startRunbookpassesrunbookRef(not just a string path) andrunIdthrough tomanager.create, and thattemplateVarsincludes bothRunIdandRunbookRef. This confirms the identity contract flows from preparation through to state persistence. Good use ofbrandRunIdForTestfor type-safe test data.
1910-1984: Cross-agent orphan adoption rejection is correctly tested.This test validates a security-critical behavior: when an orphan run's persisted linkage doesn't match the incoming claim's linkage (different
parentRunId), the claim is rejected with'linkage-mismatch'. The assertionexpect(mockManager.update).not.toHaveBeenCalled()confirms the orphan state is not modified. Good use of the typedDelegationLinkagefororphanLinkage.
2375-2419: Child runbook resolution failure uses RunbookRef-based error path.The test now uses
resolveRunbookRefreturning{ ok: false, reason: 'file-missing' }rather thanresolveRunbookFilereturningnull. This reflects the updated resolution model where delegation flows resolve via RunbookRef. The structured failure withreason: 'prepare-failed'andcode: 'RUNBOOK_NOT_FOUND'provides actionable error information.
2522-2578: buildContextVars and buildTemplateVars tests cover type preservation.These tests validate that
context.vars.*aliases preserve various JSON types (string, number, object) rather than coercing to strings. ThebuildTemplateVarstests also confirm correct merge precedence (local overrides inherited) and that inherited context vars with nested paths are preserved.
| // 4f. Prepare child runbook from persisted source identity | ||
| const childRunbookRef = freshDelegation.childRunbookRef; | ||
| const childDisplayPath = freshDelegation.childRunbookPath; | ||
| const childResolution = await resolveRunbookRef(cwd, childRunbookRef); | ||
| if (!childResolution.ok) { | ||
| if (childResolution.reason === 'plugin-context-missing') { | ||
| return { | ||
| ok: false, | ||
| reason: 'prepare-failed', | ||
| runbook: childDisplayPath, | ||
| code: 'RUNBOOK_REF_RESOLUTION_ERROR', | ||
| cause: `Plugin runbook context is unavailable for ${childRunbookRef.source}:${childRunbookRef.path}. Set CLAUDE_PLUGIN_ROOT or install the Rundown Claude Code plugin alongside the CLI.`, | ||
| details: { runbook: childDisplayPath }, | ||
| }; | ||
| } | ||
| return { | ||
| ok: false, | ||
| reason: 'prepare-failed', | ||
| runbook: childDisplayPath, | ||
| code: 'RUNBOOK_NOT_FOUND', | ||
| cause: `Runbook not found: ${childRunbookRef.source}:${childRunbookRef.path}`, | ||
| details: { runbook: childDisplayPath }, | ||
| }; | ||
| } | ||
| const childResolved = childResolution.resolved; | ||
|
|
||
| const prepResult = await prepareResolvedRunnableRunbook( | ||
| { | ||
| resolved: childResolved, | ||
| runbookRef: childRunbookRef, | ||
| displayName: childDisplayPath, | ||
| }, | ||
| inputOpts, | ||
| cwd, | ||
| { inheritedContextVars, inheritedUserVars }, | ||
| ); |
There was a problem hiding this comment.
Fail closed on pre-RunbookRef delegation state.
This path assumes every persisted delegation already has childRunbookRef. If an older .rundown/runs parent is resumed and claimed after upgrade, resolveRunbookRef(cwd, childRunbookRef) will receive undefined and escape the no-migration contract as a runtime blow-up or opaque prepare failure. Add an explicit stale-state check here and tell the operator to complete/stop/prune the parent before retrying.
Suggested fix
const childRunbookRef = freshDelegation.childRunbookRef;
const childDisplayPath = freshDelegation.childRunbookPath;
+ if (childRunbookRef === undefined) {
+ return {
+ ok: false,
+ reason: 'prepare-failed',
+ runbook: childDisplayPath,
+ code: 'RUNBOOK_REF_RESOLUTION_ERROR',
+ cause:
+ 'This delegation was created by an older Rundown version. Complete, stop, or prune the parent run before claiming this token.',
+ details: { runbook: childDisplayPath },
+ };
+ }
const childResolution = await resolveRunbookRef(cwd, childRunbookRef);As per coding guidelines, "Warn if files that define persisted runbook state, state snapshots, lifecycle fields, variable storage, delegation state, or schemaVersion behavior change and the PR also silently migrates existing .rundown/runs/ data".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 4f. Prepare child runbook from persisted source identity | |
| const childRunbookRef = freshDelegation.childRunbookRef; | |
| const childDisplayPath = freshDelegation.childRunbookPath; | |
| const childResolution = await resolveRunbookRef(cwd, childRunbookRef); | |
| if (!childResolution.ok) { | |
| if (childResolution.reason === 'plugin-context-missing') { | |
| return { | |
| ok: false, | |
| reason: 'prepare-failed', | |
| runbook: childDisplayPath, | |
| code: 'RUNBOOK_REF_RESOLUTION_ERROR', | |
| cause: `Plugin runbook context is unavailable for ${childRunbookRef.source}:${childRunbookRef.path}. Set CLAUDE_PLUGIN_ROOT or install the Rundown Claude Code plugin alongside the CLI.`, | |
| details: { runbook: childDisplayPath }, | |
| }; | |
| } | |
| return { | |
| ok: false, | |
| reason: 'prepare-failed', | |
| runbook: childDisplayPath, | |
| code: 'RUNBOOK_NOT_FOUND', | |
| cause: `Runbook not found: ${childRunbookRef.source}:${childRunbookRef.path}`, | |
| details: { runbook: childDisplayPath }, | |
| }; | |
| } | |
| const childResolved = childResolution.resolved; | |
| const prepResult = await prepareResolvedRunnableRunbook( | |
| { | |
| resolved: childResolved, | |
| runbookRef: childRunbookRef, | |
| displayName: childDisplayPath, | |
| }, | |
| inputOpts, | |
| cwd, | |
| { inheritedContextVars, inheritedUserVars }, | |
| ); | |
| // 4f. Prepare child runbook from persisted source identity | |
| const childRunbookRef = freshDelegation.childRunbookRef; | |
| const childDisplayPath = freshDelegation.childRunbookPath; | |
| if (childRunbookRef === undefined) { | |
| return { | |
| ok: false, | |
| reason: 'prepare-failed', | |
| runbook: childDisplayPath, | |
| code: 'RUNBOOK_REF_RESOLUTION_ERROR', | |
| cause: | |
| 'This delegation was created by an older Rundown version. Complete, stop, or prune the parent run before claiming this token.', | |
| details: { runbook: childDisplayPath }, | |
| }; | |
| } | |
| const childResolution = await resolveRunbookRef(cwd, childRunbookRef); | |
| if (!childResolution.ok) { | |
| if (childResolution.reason === 'plugin-context-missing') { | |
| return { | |
| ok: false, | |
| reason: 'prepare-failed', | |
| runbook: childDisplayPath, | |
| code: 'RUNBOOK_REF_RESOLUTION_ERROR', | |
| cause: `Plugin runbook context is unavailable for ${childRunbookRef.source}:${childRunbookRef.path}. Set CLAUDE_PLUGIN_ROOT or install the Rundown Claude Code plugin alongside the CLI.`, | |
| details: { runbook: childDisplayPath }, | |
| }; | |
| } | |
| return { | |
| ok: false, | |
| reason: 'prepare-failed', | |
| runbook: childDisplayPath, | |
| code: 'RUNBOOK_NOT_FOUND', | |
| cause: `Runbook not found: ${childRunbookRef.source}:${childRunbookRef.path}`, | |
| details: { runbook: childDisplayPath }, | |
| }; | |
| } | |
| const childResolved = childResolution.resolved; | |
| const prepResult = await prepareResolvedRunnableRunbook( | |
| { | |
| resolved: childResolved, | |
| runbookRef: childRunbookRef, | |
| displayName: childDisplayPath, | |
| }, | |
| inputOpts, | |
| cwd, | |
| { inheritedContextVars, inheritedUserVars }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/helpers/runbook-pipeline.ts` around lines 1607 - 1642, The
code assumes freshDelegation.childRunbookRef is always defined before calling
resolveRunbookRef; add an explicit check on freshDelegation.childRunbookRef (or
the local childRunbookRef) immediately before resolveRunbookRef and fail with a
clear "stale delegation" prepare-failed response if it's undefined — e.g.,
return { ok:false, reason:'prepare-failed', runbook: childDisplayPath,
code:'STALE_DELEGATION', cause: 'Delegation refers to missing childRunbookRef;
resume/complete/prune the parent run before retrying', details:{ runbook:
childDisplayPath } } — then only call resolveRunbookRef and later
prepareResolvedRunnableRunbook when childRunbookRef is present. Ensure you
reference freshDelegation.childRunbookRef, childRunbookRef, resolveRunbookRef,
and prepareResolvedRunnableRunbook in the change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/__tests__/services/variable-discovery.test.ts`:
- Around line 540-557: The test currently overwrites process.env[envKey] and
always deletes it in the finally block, which can clobber pre-existing values;
update the test around envKey and resolveVariables to save the original value
(e.g., const original = process.env[envKey]) before setting process.env[envKey]
= 'shadow', then in the finally block restore the original: if (original ===
undefined) delete process.env[envKey] else process.env[envKey] = original,
ensuring test isolation and no loss of externally provided RD_INPUT_* values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d865330e-5fc7-4bf0-bcd6-75438f59215b
📒 Files selected for processing (1)
packages/cli/__tests__/services/variable-discovery.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: coverage
- GitHub Check: test (24)
- GitHub Check: lint-typed (24)
- GitHub Check: playwright
- GitHub Check: scenarios
- GitHub Check: analyze
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use clear, descriptive variable and function names that convey intent
Add comments for complex logic and non-obvious code sections
Use async/await over callbacks and promise chains
Use const by default, let for variables that need reassignment, avoid var
Use template literals instead of string concatenation
Files:
packages/cli/__tests__/services/variable-discovery.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety; define explicit types instead of relying on inference
**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript
UseisError(),isNodeError(), orgetErrorMessage()from@rundown-org/core(orpackages/claude-code-plugin/src/shared/errors.tsinside the plugin) — never callError.isError()directly
All exported TypeScript symbols must have TSDoc documentation including description,@paramfor all parameters,@returnsif non-void, and@throwsif exceptions are possible
Use discriminated unions and type narrowing to make invalid states unrepresentable. Guards should express domain conditions through typed return values, never raw action-type string checks.
Use XState's native event system and state graph structure instead of creating artificial state identifiers (like~channelprefixes) for synthetic IDs.
Files:
packages/cli/__tests__/services/variable-discovery.test.ts
**/*.test.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Write unit tests with clear arrange-act-assert structure
Files:
packages/cli/__tests__/services/variable-discovery.test.ts
**/*.{ts,tsx,js,mjs,cjs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,mjs,cjs}: Use camelCase for variable names in TypeScript/JavaScript
Always use async/await for promises in TypeScript/JavaScript
Variable names must match the pattern/^[a-zA-Z_][a-zA-Z0-9_]*$/(alphanumeric and underscore, starting with letter or underscore)
Files:
packages/cli/__tests__/services/variable-discovery.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Mock injected core services structurally in non-core tests using object-shaped service doubles instead of constructing real core services from mocked modules
Files:
packages/cli/__tests__/services/variable-discovery.test.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (Custom checks)
**/*.{ts,tsx,js}: Warn if CLI JSON output, command names, or --schema behavior change without corresponding OutputEmitter usage, output schema mapping updates, and schema coverage tests
Pass CLI schema coverage if the PR does not touch CLI output behavior
Pass CLI schema coverage if schema source, CLI exports, and tests are updated together
Warn if files that define persisted runbook state, state snapshots, lifecycle fields, variable storage, delegation state, or schemaVersion behavior change and the PR also silently migrates existing .rundown/runs/ data
Pass persisted state compatibility if the PR does not touch persisted state behavior
Pass persisted state compatibility if the PR preserves compatibility or detects stale state and requires completion, stop, or prune before restart
Warn if policy, sandbox, command execution, file data sources, path resolution, or environment handling changes widen command, path, or environment capabilities without corresponding tests or docs/reference/security.md updates
Pass security policy review if the PR does not touch these areas
Files:
packages/cli/__tests__/services/variable-discovery.test.ts
{**/*.test.{ts,tsx,js},docs/reference/security.md}
📄 CodeRabbit inference engine (Custom checks)
Pass security policy review if the PR includes tests or documentation showing traversal, symlink escape, deny precedence, env leakage, shell parsing, and fail-closed behavior were preserved
Files:
packages/cli/__tests__/services/variable-discovery.test.ts
packages/cli/**/*.ts
⚙️ CodeRabbit configuration file
packages/cli/**/*.ts: CLI package. Focus on:
- OutputEmitter usage for new CLI commands.
- JSON output by default and --text for human-readable output.
- UPPERCASE table headers, 2-space column separators, left-aligned text, and right-aligned numbers.
- Proper error handling, command option validation, and user-facing messages.
- If command JSON output changes, verify schema mappings and tests in packages/cli/src/schemas, packages/core/src/output, and packages/cli/tests.
Files:
packages/cli/__tests__/services/variable-discovery.test.ts
**/__tests__/**/*.ts
⚙️ CodeRabbit configuration file
**/__tests__/**/*.ts: Test files. Focus on:
- Use Error.isError() instead of instanceof Error except for same-realm custom error classes.
- Mock passthrough pattern: pass through real functions that do not need mocking.
- Test isolation, meaningful assertions, edge cases, and regression coverage for changed behavior.
- Prefer targeted verification commands that match the package touched by the PR.
Files:
packages/cli/__tests__/services/variable-discovery.test.ts
🧠 Learnings (4)
📚 Learning: 2026-03-13T07:37:13.038Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 102
File: packages/cli/__tests__/commands/fail.test.ts:134-141
Timestamp: 2026-03-13T07:37:13.038Z
Learning: In the tobyhede/rundown repository, ensure that ErrorResponseSchema in packages/core/src/output/zod-schemas.ts uses command: z.string().optional() (renamed from action) to reflect NoActiveRunbookOutput.action → command across the codebase. Update and verify all references to this field, including related code paths such as OutputEmitter.noActiveRunbook() and json-renderer.ts. During reviews, check that tests (e.g., packages/cli/__tests__/commands/fail.test.ts) align with the new field name and that any expectations or mocks reference command rather than action.
Applied to files:
packages/cli/__tests__/services/variable-discovery.test.ts
📚 Learning: 2026-04-06T04:20:01.297Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 177
File: packages/claude-code-plugin/runbooks/planning/write-plan.runbook.md:24-24
Timestamp: 2026-04-06T04:20:01.297Z
Learning: In this repository (tobyhede/rundown), `CLAUDE_PLUGIN_ROOT` is part of a published contract and is guaranteed to already include a trailing `/` (enforced since PR `#174`). Therefore, when reviewing code, do not flag path concatenations that join the variable directly to the next path segment (e.g., `{{ CLAUDE_PLUGIN_ROOT }}schemas/plan.schema.json`) as missing a separator—this resolves correctly because the trailing slash is included in the variable value.
Applied to files:
packages/cli/__tests__/services/variable-discovery.test.ts
📚 Learning: 2026-05-03T01:45:03.732Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 255
File: packages/cli/__tests__/commands/status.test.ts:239-249
Timestamp: 2026-05-03T01:45:03.732Z
Learning: In tobyhede/rundown’s status output logic (notably `buildStashedStatus` in `packages/cli`/`packages/core`), when a stashed runbook has `parentLinkage.kind === 'delegation'`, the generated status must omit the `vars` key for delegation-claimed children. Specifically, `rd status` (without `--claim-id`) must not expose inherited variable contents for these delegation-claimed descendants (e.g., ensure output does not contain sensitive values). Only callers using `rd status --claim-id <claimId>` (the capability-holder) are allowed to see the `vars` field for stashed delegated children. This guards against production data leaks; preserve/test this isolation behavior (as fixed in PR `#255`, commit dfba8464).
Applied to files:
packages/cli/__tests__/services/variable-discovery.test.ts
📚 Learning: 2026-05-03T01:45:37.414Z
Learnt from: tobyhede
Repo: tobyhede/rundown PR: 255
File: packages/cli/__tests__/commands/stop.test.ts:203-211
Timestamp: 2026-05-03T01:45:37.414Z
Learning: When reviewing code/tests in this repo that handle errors emitted via `OutputEmitter.error(message, code, details)`, do not expect or suggest a `command` field in the emitted error envelope. The error response shape contains only `kind`, `code`, and `error`/message fields (no `command`). If a `command` field is required for error envelopes, it should be implemented by updating `OutputEmitter` itself (as a follow-up), rather than by asserting `response.command` in callers/tests.
Applied to files:
packages/cli/__tests__/services/variable-discovery.test.ts
🔇 Additional comments (2)
packages/cli/__tests__/services/variable-discovery.test.ts (2)
183-190: Good runtime-identity contract pinning.This cleanly verifies identity keys stay in
BUILTIN_VARIABLESwhile remaining excluded from discovery output.
510-535: Strong multi-ingress rejection coverage for identity keys.The new matrix across
--input,--input-json, and--input-fileis a solid regression guard for runtime identity shadowing.
…ommandWithEnv (T4)
1 participant
Summary
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Testing
npm run verifyTargeted tests or checks:
Review Notes
Check all that apply:
docs/spec/language.md,docs/spec/grammar.md, andrunbook fixtures were checked.
symlink escape, deny precedence, env leakage, and fail-closed behavior were checked.